Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 11 commits into from

Conversation

@kazunoriogata
Copy link

@kazunoriogata kazunoriogata commented Jan 15, 2021

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8259822: [PPC64] Support the prefixed instruction format added in POWER10

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/2095/head:pull/2095
$ git checkout pull/2095

Update a local copy of the PR:
$ git checkout pull/2095
$ git pull https://git.openjdk.java.net/jdk pull/2095/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2095

View PR using the GUI difftool:
$ git pr show -t 2095

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/2095.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 15, 2021

👋 Welcome back ogatak! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jan 15, 2021

@kazunoriogata The following label will be automatically applied to this pull request:

  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

Loading

@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Jan 15, 2021

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

Loading

@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Jan 15, 2021

/issue add JDK-8259822

Loading

@openjdk openjdk bot changed the title [PPC64] Support the prefixed instruction format added in POWER10 8259822: [PPC64] Support the prefixed instruction format added in POWER10 Jan 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 15, 2021

@kazunoriogata The primary solved issue for a PR is set through the PR title. Since the current title does not contain an issue reference, it will now be updated.

Loading

@openjdk openjdk bot added the rfr label Jan 15, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 15, 2021

Loading

Copy link

@CoreyAshford CoreyAshford left a comment

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

Loading

PREFIX_OPCODE_TYPEx0_MASK = PREFIX_OPCODE_TYPE_MASK | ( 1u << PRE_ST1_SHIFT),
PREFIX_OPCODE_TYPEx1_MASK = PREFIX_OPCODE_TYPE_MASK | (15u << PRE_ST4_SHIFT),

//Masks for each instructions

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a space after the //

Loading

@@ -1081,6 +1109,20 @@ class Assembler : public AbstractAssembler {
static int inv_bo_field(int x) { return inv_opp_u_field(x, 10, 6); }
static int inv_bi_field(int x) { return inv_opp_u_field(x, 15, 11); }

// support to extended opcodes (prefixed instructions) introduced by POWER10

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Author

@kazunoriogata kazunoriogata Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, sounds good.

Loading

@@ -1202,6 +1244,24 @@ class Assembler : public AbstractAssembler {
static int vcmp_rc( int x) { return opp_u_field(x, 21, 21); } // for vcmp* instructions
static int xxsplt_uim(int x) { return opp_u_field(x, 15, 14); } // for xxsplt* instructions

// support to extended opcodes (prefixed instructions) introduced by POWER10

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Support to->for extended opcodes ...

Loading

@@ -1245,6 +1305,22 @@ class Assembler : public AbstractAssembler {
static inline int hi16_signed( int x) { return (int)(int16_t)(x >> 16); }
static inline int lo16_unsigned(int x) { return x & 0xffff; }

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here.

Loading

return false;
}
}
static bool is_pli(const int* p) { return is_paddi(p, true); }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Author

@kazunoriogata kazunoriogata Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

// Load 34-bit long constant using prefixed addi. No constant pool entries required.
instruct loadConL34(iRegLdst dst, immL34 src) %{
match(Set dst src);
ins_cost(DEFAULT_COST+1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Author

@kazunoriogata kazunoriogata Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think we can't use a predicate for Set dst src. It's fine to have it in the operand.
Would be nice to describe this in the comment.

Loading

inline void Assembler::pla( Register d, Register a, long si34) { Assembler::paddi( d, a, si34, false); }
inline void Assembler::psubi(Register d, Register a, long si34) { Assembler::paddi( d, a, -si34, false); }

inline void Assembler::pli_or_li(Register d, long si34) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defined but not used.

Loading

Copy link
Author

@kazunoriogata kazunoriogata Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

emit_int32( PADDI_SUFFIX_OPCODE | rt(d) | ra(a) | d1_eo(si34));
}

inline void Assembler::paddi_or_addi(Register d, Register a, long si34) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defined but not used.

Loading

Copy link
Author

@kazunoriogata kazunoriogata Jan 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

#ifdef COMPILER2
Thread* thread = Thread::current();

if(!thread->is_Compiler_thread()) return false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add space after "if"

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jan 26, 2021

@kazunoriogata this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout paddi
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

Loading

@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Jan 26, 2021

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

Loading

@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Feb 1, 2021

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

Loading

Copy link

@CoreyAshford CoreyAshford left a comment

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

Loading


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add space after 'if'

Loading

// Prefixed instructions, introduced by POWER10
inline void Assembler::paddi( Register d, Register a, long si34, bool r = false) {
assert(a != R0 || r, "r0 not allowed, unless R is set (CIA relative)");
paddi_r0ok( d, a, si34, r);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, please remove extra spaces.

Loading

static int is_paddi(const int* p, bool is_pli = false) {
int32_t* p_inst = (int32_t*)p;

if (is_aligned(reinterpret_cast<uintptr_t>(p_inst+1), 64) && is_nop(*p_inst)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Loading

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

// Load 34-bit long constant using prefixed addi. No constant pool entries required.
instruct loadConL34(iRegLdst dst, immL34 src) %{
match(Set dst src);
ins_cost(DEFAULT_COST+1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

inline void pla( Register d, long si34);
inline void pla( Register d, Register a, long si34);
inline void psubi(Register d, Register a, long si34);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pla and psubi are defined but not used.

Loading

@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Feb 2, 2021

@CoreyAshford Thank you for your review. I went through my changes to check spacing, as well as removed unused pla and psubi as you pointed out.

Loading

Copy link

@CoreyAshford CoreyAshford left a comment

Looks good! Thanks for the additional formatting cleanup

Loading

@TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Feb 3, 2021

Sorry that I didn't review it, yet. I didn't have enough time.

Please note that C2 already has a mechanism to handle alignment:

Insert "ins_alignment(n);" into the instruct. This allows C2 to insert up to n-1 nop instructions.
Add a "compute_padding" function to determine the actual number of nops to insert given the current offset.
They are used in s390.ad. Please take a look.

Loading

@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Feb 5, 2021

@CoreyAshford Thank you for your review.

@TheRealMDoerr Thank you for your suggestion. I think I understand the mechanism. I'll update my patch after verifing my change and running jtreg.

Loading

@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Mar 23, 2021

@TheRealMDoerr Sorry to be late to update the patch. It took time to debug my careless mistakes...

I updated the patch with using ins_alignment and verified there is no additional errors in jtreg tier1 tests. Although there were intermittent failures in VarHandleTests using weakCompareAndSet, it was the same situation as the base build, so I think it is not related to my changes.

Loading

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Thanks for contributing this and for using ins_alignment. Update needs further changes.

Loading

return false;
#endif
}

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is still needed. Please revert changes to this file.

Loading

@@ -1263,11 +1339,14 @@ class Assembler : public AbstractAssembler {
return (0 == addr % a);
}

static bool in_scratch_emit_size();

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not be needed any more.

Loading

// Prefixed instructions, introduced by POWER10
inline void Assembler::paddi( Register d, Register a, long si34, bool r = false) {
assert(a != R0 || r, "r0 not allowed, unless R is set (CIA relative)");
paddi_r0ok( d, a, si34, r);
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, please remove extra spaces.

Loading

int loadConI32Node::compute_padding(int current_offset) const {
assert(PowerArchitecturePPC64 >= 10 && (CodeEntryAlignment & 63) == 0,
"Code buffer must be aligned to a multiple of 64 byte");
if (is_aligned(current_offset + BytesPerInstWord, 64) || Assembler::in_scratch_emit_size()) {
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compute_padding is not called during scratch emit. So please remove Assembler::in_scratch_emit_size().

Loading

__ pli($dst$$Register, $src$$constant);
%}
ins_pipe(pipe_class_default);
ins_alignment(4);
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ins_alignment is specified in number of nops, not number of Bytes. So it should be 2 which means that up to 1 nop can get inserted.

Loading

// Load 34-bit long constant using prefixed addi. No constant pool entries required.
instruct loadConL34(iRegLdst dst, immL34 src) %{
match(Set dst src);
ins_cost(DEFAULT_COST+1);
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think we can't use a predicate for Set dst src. It's fine to have it in the operand.
Would be nice to describe this in the comment.

Loading

@@ -5912,7 +6020,7 @@ instruct loadConL(iRegLdst dst, immL src, iRegLdst toc) %{
ins_field_cbuf_insts_offset(int);

format %{ "LD $dst, offset, $toc \t// load long $src from TOC" %}
size(4);
size(12);
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you changing this? Looks wrong. I can only see 1 4 Byte instruction.

Loading

static int is_paddi(const int* p, bool is_pli = false) {
int32_t* p_inst = (int32_t*)p;

if (is_aligned(reinterpret_cast<uintptr_t>(p_inst+1), 64) && is_nop(*p_inst)) {
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need to skip over the nop? I think this should no longer happen.

Loading

@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Mar 24, 2021

@TheRealMDoerr Thank you for your review. I updated the patch based on your comments. As a result of this, all changes in assembler_ppc.cpp were reverted.

@@ -5912,7 +6020,7 @@ instruct loadConL(iRegLdst dst, immL src, iRegLdst toc) %{
   ins_field_cbuf_insts_offset(int);
   format %{ "LD      $dst, offset, $toc \t// load long $src from TOC" %}
 - size(4);
 + size(12);

I picked up a diff for other changes by my mistake. Thank you for pointing this out.

  static int is_paddi(const int* p, bool is_pli = false) {
     int32_t* p_inst = (int32_t*)p;

     if (is_aligned(reinterpret_cast<uintptr_t>(p_inst+1), 64) && is_nop(*p_inst)) {

TheRealMDoerr
Do we still need to skip over the nop? I think this should no longer happen.

I think it is still useful when JIT scans a sequence of generated code like MacroAssembler::get_const(). Assuming a generated sequence:

  paddi rA, const32a(r0)
  nop
  paddi rB, const32b(r0)

it would be convenient if get_const() can check the next instruction after the first paddi without taking care of the nop, and it can start scanning from the nest instruction address (= label+8).

Loading

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Thanks for the update. Looks basically good to me, but the merge conflict needs to get resolved. Also, please check for unused code.

Loading

* Copyright (c) 2002, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012, 2020 SAP SE. All rights reserved.
* Copyright (c) 2002, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2012, 2021 SAP SE. All rights reserved.
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Copyright year change in this file was already done by another change. You need to revert it in order to resolve the merge conflict.

Loading

}

static inline int hi18_signed( int x) { return hi16_signed(x); }
static inline int hi18_signed(long x) { return (int)((x << 30) >> 46); }
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sees like they are unused and should get removed.

Loading

@@ -1268,6 +1344,7 @@ class Assembler : public AbstractAssembler {
}

inline void emit_int32(int); // shadows AbstractAssembler::emit_int32
inline void emit_prefix(int); // emit prefix word only (and a nop to skip 64-byte boundary)
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused.

Loading

} else {
return false;
}
}
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't see any usage of it. As Corey stated, unused code should better get removed because it doesn't get tested and may break unless you're planning to use it in the near future or need it for debugging purposes.

Loading

@openjdk openjdk bot removed the merge-conflict label Mar 24, 2021
@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Mar 24, 2021

@TheRealMDoerr Thank you for your review.

TheRealMDoerr
I can't see any usage of it. As Corey stated, unused code should better get removed because it doesn't get tested and may break unless you're planning to use it in the near future or need it for debugging purposes.

I see. These code were locally used for supporting paddi in load_const()/get_const()/patch_const(), so I intended to use it soon. However, it still has some errors. So I'll move these changes to the patch after fixing the errors.

TheRealMDoerr
The Copyright year change in this file was already done by another change. You need to revert it in order to resolve the merge conflict.

Thank you for the tip! I intended to merge to the latest code base when all other changes are reviewed, but reverting the lines is much easier to get the same result.

Loading

Copy link

@CoreyAshford CoreyAshford left a comment

No functional comments. Just some minor grammar things.

Loading

// (See Section 1.6 of Power ISA Version 3.1)
int loadConI32Node::compute_padding(int current_offset) const {
assert(PowerArchitecturePPC64 >= 10 && (CodeEntryAlignment & 63) == 0,
"Code buffer must be aligned to a multiple of 64 byte");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... multiple of 64 bytes (plural)

Loading


int loadConL34Node::compute_padding(int current_offset) const {
assert(PowerArchitecturePPC64 >= 10 && (CodeEntryAlignment & 63) == 0,
"Code buffer must be aligned to a multiple of 64 byte");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.. bytes

Loading


int addI_reg_imm32Node::compute_padding(int current_offset) const {
assert(PowerArchitecturePPC64 >= 10 && (CodeEntryAlignment & 63) == 0,
"Code buffer must be aligned to a multiple of 64 byte");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... bytes

Loading


int addL_reg_imm34Node::compute_padding(int current_offset) const {
assert(PowerArchitecturePPC64 >= 10 && (CodeEntryAlignment & 63) == 0,
"Code buffer must be aligned to a multiple of 64 byte");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... bytes

Loading


int addP_reg_imm34Node::compute_padding(int current_offset) const {
assert(PowerArchitecturePPC64 >= 10 && (CodeEntryAlignment & 63) == 0,
"Code buffer must be aligned to a multiple of 64 byte");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... bytes.

Since this string is used so many times, I almost feel like it ought to be factored out.

Loading


int cmprb_Whitespace_reg_reg_prefixedNode::compute_padding(int current_offset) const {
assert(PowerArchitecturePPC64 >= 10 && (CodeEntryAlignment & 63) == 0,
"Code buffer must be aligned to a multiple of 64 byte");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... bytes

Loading

instruct loadConI32(iRegIdst dst, immI32 src) %{
match(Set dst src);
// This macro is valid only in Power 10 and up, but adding the following predicate here
// caused build error. So we comment it out for now.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small grammar issue:

// caused a build error , so we comment it out for now.

Loading

instruct loadConL34(iRegLdst dst, immL34 src) %{
match(Set dst src);
// This macro is valid only in Power 10 and up, but adding the following predicate here
// caused build error. So we comment it out for now.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// caused a build error , so we comment it out for now.

Loading

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr Mar 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// caused a build error , so we comment it out for now.

Better would be, "so we only check it in the operand".

Loading

@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Mar 25, 2021

@CoreyAshford Thank you for your comment. (I want an automatic grammar checker in github...)

Since this string is used so many times, I almost feel like it ought to be factored out.

I factored out the function bodies of *Node::compute_padding() to a static function, since they are just duplicated.

Loading

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Looks good to me, now. Is the latest version substantially tested? We need to rely on IBMs testing because nobody else has Power10.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 25, 2021

@kazunoriogata This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: cashford, mdoerr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1289 new commits pushed to the master branch:

  • 5bd6c74: 8236127: Use value of --icon CLI option to set icon for exe installers
  • 81d35e4: 8264063: Outer Safepoint poll load should not reference the head of inner strip mined loop.
  • 04fa1ed: 8264848: [macos] libjvm.dylib linker warning due to macOS version mismatch
  • 214d6e2: 8263506: Make sun.net.httpserver.UnmodifiableHeaders unmodifiable
  • af13c64: 8264711: More runtime TRAPS cleanups
  • 3aec2d9: 8264718: Shenandoah: enable string deduplication during root scanning
  • 255afbe: 8264672: runtime/ParallelLoad/ParallelSuperTest.java timed out
  • ec599da: 8264633: Add missing logging to PlatformRecording#stop
  • e89542f: 8264352: AArch64: Optimize vector "not/andNot" for NEON and SVE
  • 016db40: 8263907: Specification of CellRendererPane::paintComponent(..Rectangle) should clearly mention which method it delegates the call to
  • ... and 1279 more: https://git.openjdk.java.net/jdk/compare/6472104e18d697eb243f26a17fdbc642fb879867...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TheRealMDoerr) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Loading

@openjdk openjdk bot added the ready label Mar 25, 2021
@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Mar 26, 2021

Thank you for a heads-up. I've just run jtreg teir1 tests with the latest version. I'll run DaCapo, Renaissance, and SPECjbb2015 to verify the latest version.

Loading

@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Mar 31, 2021

@TheRealMDoerr I verified there was no additional errors against the base version in DaCapo, Renaissance, and SPECjbb2015. Altough some tests in DaCapo and Renaissance (e.g., DaCapo eclipse and tradesoap and Renaissance db-shootout) failed in the modified version, they also failed in the base build with the same error.

Loading

@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Apr 5, 2021

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Apr 5, 2021

@kazunoriogata
Your change (at version 8f8e00e) is now ready to be sponsored by a Committer.

Loading

@openjdk openjdk bot added the sponsor label Apr 5, 2021
@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Apr 5, 2021

@TheRealMDoerr @CoreyAshford Thank you for your review. I think this is now ready to be integrated.

@TheRealMDoerr Could you sponsor this pull request?

Loading

@TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Apr 8, 2021

Yes, I can sponsor it. Thanks for testing.
@CoreyAshford Are all your requests resolved? Do you need more time for additional tests? Please approve once you're fine with it.

Loading

@CoreyAshford
Copy link

@CoreyAshford CoreyAshford commented Apr 8, 2021

The only other thing I see is that there are a number of static functions that are defined for accessing fields (e.g. inv_st_x1), but aren't actually used yet. However, I think these are probably ok, since they are not very complex. So I will approve.

Loading

@TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Apr 9, 2021

/sponsor

Loading

@openjdk openjdk bot closed this Apr 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 9, 2021

@TheRealMDoerr @kazunoriogata Since your change was applied there have been 1302 commits pushed to the master branch:

  • a45733f: 8264779: Fix doclint warnings in java/nio
  • 3e57924: 8264885: Fix the code style of macro in aarch64_neon_ad.m4
  • 051c117: 8264923: PNGImageWriter.write_zTXt throws Exception with a typo
  • 1c6b113: 8264513: Cleanup CardTableBarrierSetC2::post_barrier
  • 666fd62: 8264881: Remove the old development option MemProfiling
  • 951f277: 8264874: Build interim-langtools for HotSpot only if Graal is enabled
  • 719f95e: 8260693: Provide the support for specifying a signer in keytool -genkeypair
  • 77b1673: 8256245: AArch64: Implement Base64 decoding intrinsic
  • 57f1e7d: 8264696: Multi-catch clause causes compiler exception because it uses the package-private supertype
  • 3d2b4cc: 8264864: Multiple byte tag not supported by ASN.1 encoding
  • ... and 1292 more: https://git.openjdk.java.net/jdk/compare/6472104e18d697eb243f26a17fdbc642fb879867...master

Your commit was automatically rebased without conflicts.

Pushed as commit f7a6c63.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

@kazunoriogata
Copy link
Author

@kazunoriogata kazunoriogata commented Apr 10, 2021

@TheRealMDoerr Thank you for sponsoring this patch.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants