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

classic Classic list List threaded Threaded
47 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Kazunori Ogata-2
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

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

Commit messages:
 - Expand tab to space
 - Revert unnecessary changes
 - Merge branch 'master' of github.com:openjdk/jdk into paddi
 - Support the prefixed instruction format added in POWER 10

Changes: https://git.openjdk.java.net/jdk/pull/2095/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2095&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8259822
  Stats: 310 lines in 4 files changed: 303 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2095.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2095/head:pull/2095

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Kazunori Ogata-2
On Fri, 15 Jan 2021 08:09:56 GMT, Kazunori Ogata <[hidden email]> 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

I ran jtreg test on POWER10 box by using "make test-tier1" and verified no additional fails.

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

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Corey Ashford-2
In reply to this post by Kazunori Ogata-2
On Fri, 15 Jan 2021 08:09:56 GMT, Kazunori Ogata <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Kazunori Ogata-2
On Thu, 21 Jan 2021 00:28:40 GMT, Corey Ashford <[hidden email]> 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
>
> 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.

I'll leave it since other comments that refer to a specific ISA implementation use Power X. (I changed POWER10 to Power 10, though.)

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

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Kazunori Ogata-2
In reply to this post by Corey Ashford-2
On Thu, 21 Jan 2021 00:58:14 GMT, Corey Ashford <[hidden email]> 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
>
> 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.

This comment was simply copied from old code that was deleted in JDK-8253029.  I removed the commend in this patch, too.

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

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Kazunori Ogata-2
In reply to this post by Corey Ashford-2
On Thu, 21 Jan 2021 01:17:58 GMT, Corey Ashford <[hidden email]> 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
>
> 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)?

"is_<instr>()" are the functions for testing instructions in the generated code and they are usually called in "assert()" or complex combination of if statements.  So if this function returns false when p points at nop, the assertions or the if-conditions need to handle the nop case and the code will become difficult to read.  Since the padding nop is a non-common case, I'd like to hide the existence of nop in this function.

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

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Kazunori Ogata-2
In reply to this post by Corey Ashford-2
On Thu, 21 Jan 2021 01:34:19 GMT, Corey Ashford <[hidden email]> 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
>
> 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.

If predicate is added, adlc fails with an error message: "Syntax Error: :ADLC does not support instruction chain rules with predicates"  I think addL_reg_imm34 allows predicate because it is not called from other rules.  Is it better to leave some comments?  (BTW, immI32 is only for POWER10 or higher.  POWER9 version uses immI16 or immI16hi.)

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

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Kazunori Ogata-2
In reply to this post by Corey Ashford-2
On Thu, 21 Jan 2021 01:19:02 GMT, Corey Ashford <[hidden email]> 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
>
> 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.

It is just intended to show a usage of the second argument of is_paddi(const int* p, bool is_pli = false).  Is it unnecessary, i.e., self-evident?

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

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Kazunori Ogata-2
In reply to this post by Kazunori Ogata-2
> 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

Kazunori Ogata has updated the pull request incrementally with one additional commit since the last revision:

  Update based on review comments

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2095/files
  - new: https://git.openjdk.java.net/jdk/pull/2095/files/05b92eaf..9a7ef64e

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2095&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2095&range=00-01

  Stats: 16 lines in 4 files changed: 0 ins; 5 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2095.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2095/head:pull/2095

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Kazunori Ogata-2
In reply to this post by Corey Ashford-2
On Thu, 21 Jan 2021 01:38:17 GMT, Corey Ashford <[hidden email]> wrote:

>> Kazunori Ogata has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Update based on review comments
>
> 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.

It is intended to show an example of an efficient set of macros/functions for a prefixed instruction. Since a non-prefixed (32-bit) instruction is more efficient if the argument fits within the operand fields because of smaller I-cache usage and no need for the padding nop, it is convenient to have a macro/function that select better format. I intended to show an example of recommended set of useful macros/functions when adding support for a prefixed instruction. I'm planning to use this function in my next patch. Is it better to move this to the next patch?

> 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.

Same reason as the next comment (paddi_or_addi), but necessity of this one may be weaker.

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

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Kazunori Ogata-2
In reply to this post by Corey Ashford-2
On Thu, 21 Jan 2021 01:53:02 GMT, Corey Ashford <[hidden email]> wrote:

>> Kazunori Ogata has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Update based on review comments
>
> This looks good overall.  I'm looking forward to being able to utilize this capability.

@CoreyAshford Thank you very much for your review.  I updated my patch based on your comment.  For some comments that I left unchanged, I replied to each of them.  Please give me your thought.

For merge conflict, I verified the conflict is only the copyright years.  I'll fix it when real changes are reviewed because pulling the latest master to resolve conflict makes this patch difficult to review.

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

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Corey Ashford-2
In reply to this post by Kazunori Ogata-2
On Tue, 26 Jan 2021 18:55:13 GMT, Kazunori Ogata <[hidden email]> wrote:

>> 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.
>
> I'll leave it since other comments that refer to a specific ISA implementation use Power X. (I changed POWER10 to Power 10, though.)

ok, sounds good.

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

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Corey Ashford-2
In reply to this post by Kazunori Ogata-2
On Tue, 26 Jan 2021 19:18:18 GMT, Kazunori Ogata <[hidden email]> wrote:

>> 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.
>
> It is just intended to show a usage of the second argument of is_paddi(const int* p, bool is_pli = false).  Is it unnecessary, i.e., self-evident?

In past open source projects I have worked on, functions that essentially are unreachable in the final product cannot be tested, so there's no way to know if they work as intended. For that reason, only code that is used and/or can be tested in the final product (e.g. via an externally-visible API) would be accepted in a commit.

I don't know if the OpenJDK project follows that rule or not.

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

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Corey Ashford-2
In reply to this post by Kazunori Ogata-2
On Tue, 26 Jan 2021 19:14:45 GMT, Kazunori Ogata <[hidden email]> wrote:

>> 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.
>
> If predicate is added, adlc fails with an error message: "Syntax Error: :ADLC does not support instruction chain rules with predicates"  I think addL_reg_imm34 allows predicate because it is not called from other rules.  Is it better to leave some comments?  (BTW, immI32 is only for POWER10 or higher.  POWER9 version uses immI16 or immI16hi.)

Hmm, I'm confused.  I don't see any other reference to loadConL34 in ppc.ad.

>> 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.
>
> Same reason as the next comment (paddi_or_addi), but necessity of this one may be weaker.

Ok.  See my other comments about unused/untestable code.

>> 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.
>
> It is intended to show an example of an efficient set of macros/functions for a prefixed instruction. Since a non-prefixed (32-bit) instruction is more efficient if the argument fits within the operand fields because of smaller I-cache usage and no need for the padding nop, it is convenient to have a macro/function that select better format. I intended to show an example of recommended set of useful macros/functions when adding support for a prefixed instruction. I'm planning to use this function in my next patch. Is it better to move this to the next patch?

It would be my preference to post code up only when it's actually used.  When you get around to using it, there's a possibility you will want to modify it a bit, add a comment, etc.   So that can be done all in one commit, instead of split across two.

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

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Corey Ashford-2
In reply to this post by Kazunori Ogata-2
On Tue, 26 Jan 2021 19:07:40 GMT, Kazunori Ogata <[hidden email]> wrote:

>> 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)?
>
> "is_<instr>()" are the functions for testing instructions in the generated code and they are usually called in "assert()" or complex combination of if statements.  So if this function returns false when p points at nop, the assertions or the if-conditions need to handle the nop case and the code will become difficult to read.  Since the padding nop is a non-common case, I'd like to hide the existence of nop in this function.

Ok, got it.  Thanks for the explanation.  Maybe an extra comment in the code saying essentially what you said here would be appropriate?

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

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Kazunori Ogata-2
In reply to this post by Kazunori Ogata-2
> 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

Kazunori Ogata has updated the pull request incrementally with one additional commit since the last revision:

  Update (2nd round) based on review comments

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2095/files
  - new: https://git.openjdk.java.net/jdk/pull/2095/files/9a7ef64e..a8770539

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2095&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2095&range=01-02

  Stats: 32 lines in 3 files changed: 6 ins; 23 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2095.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2095/head:pull/2095

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Kazunori Ogata-2
In reply to this post by Corey Ashford-2
On Thu, 21 Jan 2021 01:53:02 GMT, Corey Ashford <[hidden email]> wrote:

>> Kazunori Ogata has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Update (2nd round) based on review comments
>
> This looks good overall.  I'm looking forward to being able to utilize this capability.

@CoreyAshford Thank you for your comment.  Regarding the unused code, I agree that unused code won't be tested enough, so I removed paddi_or_addi(), pli_or_li(), and is_pli().

For the comment on predicate in loadConI32 and loadConL34, I couldn't find the reason why it caused build error.  So I added comments describing they are only for Power 10 and up but can't add predicate.  I also added comments in is_paddi() as you suggested.

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

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Corey Ashford-2
In reply to this post by Corey Ashford-2
On Wed, 27 Jan 2021 17:50:57 GMT, Corey Ashford <[hidden email]> wrote:

>> "is_<instr>()" are the functions for testing instructions in the generated code and they are usually called in "assert()" or complex combination of if statements.  So if this function returns false when p points at nop, the assertions or the if-conditions need to handle the nop case and the code will become difficult to read.  Since the padding nop is a non-common case, I'd like to hide the existence of nop in this function.
>
> Ok, got it.  Thanks for the explanation.  Maybe an extra comment in the code saying essentially what you said here would be appropriate?

Looks good!

>> If predicate is added, adlc fails with an error message: "Syntax Error: :ADLC does not support instruction chain rules with predicates"  I think addL_reg_imm34 allows predicate because it is not called from other rules.  Is it better to leave some comments?  (BTW, immI32 is only for POWER10 or higher.  POWER9 version uses immI16 or immI16hi.)
>
> Hmm, I'm confused.  I don't see any other reference to loadConL34 in ppc.ad.

I wish we knew why this predicate is causing an issue, but I guess it's not important because the operand type provides sufficient limiting of the instruct.

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

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Corey Ashford-2
In reply to this post by Kazunori Ogata-2
On Mon, 1 Feb 2021 08:28:06 GMT, Kazunori Ogata <[hidden email]> 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
>
> Kazunori Ogata has updated the pull request incrementally with one additional commit since the last revision:
>
>   Update (2nd round) based on review comments

Just a few more minor things... several defined-but-not-used functions, and some little formatting issues.

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

> 41:   // Add nop if a prefixed (two-word) instruction is going to cross a 64-byte boundary.
> 42:   // (See Section 1.6 of Power ISA Version 3.1)
> 43:   if(is_aligned(reinterpret_cast<uintptr_t>(pc()) + sizeof(int32_t), 64) ||

add space after 'if'

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

> 147: inline void Assembler::paddi(  Register d, Register a, long si34, bool r = false) {
> 148:   assert(a != R0 || r, "r0 not allowed, unless R is set (CIA relative)");
> 149:   paddi_r0ok( d, a, si34, r);

The space after the ( isn't needed here, since it's not aligning with a similar call above or below.

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

> 1320:
> 1321:   static inline int hi18_signed(  int x) { return hi16_signed(x); }
> 1322:   static inline int hi18_signed( long x) { return (int)((x << 30) >> 46); }

The extra spaces for alignment look a bit strange here.  I think it should be:
static inline int hi18_signed( int x) ...
static inline int hi18_signed(long x) ...
Basically, remove the leading space from the ( long x) one and delete one from (  int x) to match.

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

> 1387:   inline void pla(  Register d, long si34);
> 1388:   inline void pla(  Register d, Register a, long si34);
> 1389:   inline void psubi(Register d, Register a, long si34);

pla and psubi are defined but not used.

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

Changes requested by [hidden email] (no known OpenJDK username).

PR: https://git.openjdk.java.net/jdk/pull/2095
Reply | Threaded
Open this post in threaded view
|

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

Kazunori Ogata-2
In reply to this post by Kazunori Ogata-2
> 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

Kazunori Ogata has updated the pull request incrementally with one additional commit since the last revision:

  Removed pla and psubi and adjusted spacing based on review comments

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2095/files
  - new: https://git.openjdk.java.net/jdk/pull/2095/files/a8770539..9d606d67

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2095&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2095&range=02-03

  Stats: 23 lines in 3 files changed: 2 ins; 6 del; 15 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2095.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2095/head:pull/2095

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