RFR: 8259822: [PPC64] Support the prefixed instruction format added in POWER10

[Corey Ashford]

On Fri, 15 Jan 2021 08:09:56 GMT, Kazunori Ogata <ogatak at openjdk.org> wrote:

> The POWER10 processor, which implements Power ISA 3.1 [1], supports new instruction formats where an instruction takes two 32bit words.  The first word is called prefix, and the instructions with prefix are called prefixed instructions.  With more bits in opcode and operand fields, POWER10 supports larger immediate value in an operand, as well as many new instructions.
> 
> This is the first changes to handle prefixed instructions, and this adds support of prefixed addi (= paddi) instruction as an example of prefix usage.  paddi accepts 34bit immediate value, while original addi accepts 16bit value.
> 
> [1] https://ibm.ent.box.com/s/hhjfw0x0lrbtyzmiaffnbxh2fuo0fog0

This looks good overall.  I'm looking forward to being able to utilize this capability.

src/hotspot/cpu/ppc/assembler_ppc.hpp line 814:

> 812:     PREFIX_OPCODE_TYPEx1_MASK = PREFIX_OPCODE_TYPE_MASK | (15u << PRE_ST4_SHIFT),
> 813: 
> 814:     //Masks for each instructions

Add a space after the //

src/hotspot/cpu/ppc/assembler_ppc.hpp line 1112:

> 1110:   static int inv_bi_field(int x)  { return inv_opp_u_field(x, 15, 11); }
> 1111: 
> 1112:   // support to extended opcodes (prefixed instructions) introduced by POWER10

Should this be "introduced by Power ISA 3.1"?  It would be more correct, but probably inconsistent with other, similar comments.

src/hotspot/cpu/ppc/assembler_ppc.hpp line 1247:

> 1245:   static int xxsplt_uim(int        x)  { return  opp_u_field(x,             15, 14); } // for xxsplt* instructions
> 1246: 
> 1247:   // support to extended opcodes (prefixed instructions) introduced by POWER10

Support to->for extended opcodes ...

src/hotspot/cpu/ppc/assembler_ppc.hpp line 1309:

> 1307: 
> 1308:   static void set_imm18(int* instr, int s) {
> 1309:     assert(PowerArchitecturePPC64 >= 10, "Prefixed instruction is supported in POWER10 and up");

I think it should be "Prefixed instructions are supported only in Power10 and up"

src/hotspot/cpu/ppc/assembler_ppc.hpp line 1317:

> 1315: 
> 1316:   static int get_imm18(address a, int instruction_number) {
> 1317:     assert(PowerArchitecturePPC64 >= 10, "Prefixed instruction is supported in POWER10 and up");

Same comment here.

src/hotspot/cpu/ppc/assembler_ppc.hpp line 1349:

> 1347: 
> 1348:   inline void emit_int32(int);  // shadows AbstractAssembler::emit_int32
> 1349:   inline void emit_prefix(int); // emit prefix word only (and a nop to skip 64byte boundary)

64byte -> 64-byte

src/hotspot/cpu/ppc/assembler_ppc.inline.hpp line 41:

> 39:          "Unexpected primary opcode for prefix word");
> 40: 
> 41:   // Add nop if a prefixed (two-word) instruction is going to cross 64-byte boundaries.

... going to cross a 64-byte boundary.

src/hotspot/cpu/ppc/ppc.ad line 12053:

> 12051: %}
> 12052: 
> 12053: // POWER10 version, using prefixed addi to load 32bit constant

32bit -> 32-bit

src/hotspot/cpu/ppc/ppc.ad line 12060:

> 12058:   ins_cost(3 * DEFAULT_COST);
> 12059: 
> 12060:   format %{ "(nop if crossing 64byte boundary)\n\t"

64byte -> 64-byte

src/hotspot/cpu/ppc/ppc.ad line 8721:

> 8719:   ins_cost(DEFAULT_COST+1);
> 8720: 
> 8721:   format %{ "(nop if crossing 64byte boundary)\n\t"

64byte -> 64-byte

src/hotspot/cpu/ppc/ppc.ad line 5845:

> 5843:   size(12);
> 5844:   ins_encode %{
> 5845:     // TODO: PPC port $archOpcode(ppc64Opcode_paddi);

I'm not clear of the meaning of this and the other TODOs.

ppc640opcode_paddi doesn't seem to be defined anywhere.

src/hotspot/cpu/ppc/assembler_ppc.hpp line 1533:

> 1531:      int32_t* p_inst = (int32_t*)p;
> 1532: 
> 1533:      if (is_aligned(reinterpret_cast<uintptr_t>(p_inst+1), 64) && is_nop(*p_inst)) {

This test is a bit confusing.  Shouldn't is_paddi return false if p points at a nop (even if it precedes the paddi)?

src/hotspot/cpu/ppc/assembler_ppc.hpp line 1543:

> 1541:      }
> 1542:   }
> 1543:   static bool is_pli(const int* p)   { return is_paddi(p, true); }

is_pli() is defined, but not used, as far as I can tell.

src/hotspot/cpu/ppc/ppc.ad line 5925:

> 5923: instruct loadConL34(iRegLdst dst, immL34 src) %{
> 5924:   match(Set dst src);
> 5925:   ins_cost(DEFAULT_COST+1);

There's no predicate for >= POWER10.  I can see how this works because of the immL34 operand having its own predicate, but in later instructs, e.g. addL_reg_imm34 even though the operand is immI32, you still add the explicit predicate.

I'd rather there be an explicit POWER10 predicate in this instruct.

src/hotspot/cpu/ppc/assembler_ppc.inline.hpp line 209:

> 207: inline void Assembler::psubi(Register d, Register a, long si34) { Assembler::paddi( d, a, -si34, false); }
> 208: 
> 209: inline void Assembler::pli_or_li(Register d, long si34) {

Defined but not used.

src/hotspot/cpu/ppc/assembler_ppc.inline.hpp line 157:

> 155: }
> 156: 
> 157: inline void Assembler::paddi_or_addi(Register d, Register a, long si34) {

defined but not used.

src/hotspot/cpu/ppc/assembler_ppc.cpp line 63:

> 61:   Thread* thread = Thread::current();
> 62: 
> 63:   if(!thread->is_Compiler_thread())       return false;

add space after "if"

-------------

PR: https://git.openjdk.java.net/jdk/pull/2095