8330844: Add aliases for conditional jumps and additional instruction forms for x86 #18893

asgibbons · 2024-04-22T16:20:39Z

Adding infrastructure for JDK-8320448. Aliasing conditional jump instructions; adding some x86 instructions.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8330844: Add aliases for conditional jumps and additional instruction forms for x86 (Enhancement - P4)

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to f7d7f7de
Emanuel Peter (@eme64 - Reviewer) ⚠️ Review applies to fe5f3060
Sandhya Viswanathan (@sviswa7 - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18893/head:pull/18893
$ git checkout pull/18893

Update a local copy of the PR:
$ git checkout pull/18893
$ git pull https://git.openjdk.org/jdk.git pull/18893/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18893

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18893.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-22T16:21:35Z

👋 Welcome back sgibbons! 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.

openjdk · 2024-04-22T16:22:19Z

@asgibbons 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:

8330844: Add aliases for conditional jumps and additional instruction forms for x86

Reviewed-by: kvn, epeter, sviswanathan

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 30 new commits pushed to the master branch:

09b8809: 8327289: Remove unused PrintMethodFlushingStatistics option
9cc163a: 8330178: Clean up non-standard use of /** comments in java.base
88a5dce: 8330805: ARM32 build is broken after JDK-8139457
7157eea: 8327290: Remove unused notproduct option TraceInvocationCounterOverflow
b4cea70: 8330693: Generational ZGC: Simplify ZAddress::finalizable_good and ZAddress::mark_good
412e306: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
b07e153: 8330362: G1: Inline offset array element accessor in G1BlockOffsetTable
b6518a5: 8329417: Remove objects with no pointers from relocation bitmap
383fe6e: 8330388: Remove invokedynamic cache index encoding
2ea8926: 8330961: Remove redundant public specifier in ModRefBarrierSet
... and 20 more: https://git.openjdk.org/jdk/compare/936a47d7d68c7305993f68db844dc76006b674b1...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 (@vnkozlov, @eme64, @sviswa7) 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).

openjdk · 2024-04-22T16:22:47Z

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

hotspot

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.

asgibbons · 2024-04-22T16:23:32Z

/label hotspot-compiler

openjdk · 2024-04-22T16:24:04Z

@asgibbons
The hotspot-compiler label was successfully added.

asgibbons · 2024-04-22T16:24:53Z

This is a precursor for JDK-8320448, essentially adding infrastructure requirements for that algorithm.

mlbridge · 2024-04-22T16:27:48Z

Webrevs

vnkozlov · 2024-04-22T18:34:41Z

making arrays_equals accessible from stubs

I am not sure I understand why you need to move it. Your changes for JDK-8320448 shows that new code is used only by C2. You can move your new code in stubGenerator_x86_64.cpp into the part under #ifdef COMPILER2. And code in stubGenerator_x86_64_string.cpp could be put under this #ifdef too.

asgibbons · 2024-04-22T20:48:23Z

Adding the #ifdef COMPILER2 in stubGenerator_x86_64_string.cpp allows for good compilation for JDK-8320448, so I can undo the move. Thanks for spotting that.

vnkozlov · 2024-04-22T21:33:43Z

Can you also remove changes in arrays_equals from this PR? It is fine to have them in JDK-8320448 changes.

vnkozlov · 2024-04-22T21:35:05Z

src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp

+  assert((!expand_ary2) || ((expand_ary2) && (UseAVX == 2)),
+         "Expansion only implemented for AVX2");


BTW, the check in assert could be simplified: (!expand_ary2 || UseAVX == 2)

I thought this would make the intent explicitly clear.

asgibbons · 2024-04-22T21:42:27Z

A large part of this PR was to lessen the burden of reviewing JDK-8320448 changes. Am I hearing you say that this approach is not desired? The other PR is a big review and I was hoping to piecemeal some non-core algorithm changes in to make the review easier.

It is, of course, trivial to revert the change to arrays_equals. Please let me know your final decision. Thanks.

vnkozlov · 2024-04-22T21:52:53Z

A large part of this PR was to lessen the burden of reviewing JDK-8320448 changes. Am I hearing you say that this approach is not desired? The other PR is a big review and I was hoping to piecemeal some non-core algorithm changes in to make the review easier.

It is, of course, trivial to revert the change to arrays_equals. Please let me know your final decision. Thanks.

I am for splitting big PRs if possible. And you are not limited how many self-containing sub-PRs you can create.
But each PR should address one issue for easy review and testing.

I consider this PR should address what in its title: aliases for jump instructions and adding missing cmp/jump instructions (which is related).

Any changes to not related code, like arrays_equals, do not belong here. It could be separate sub-PR or even followup PR.

asgibbons · 2024-04-22T22:07:54Z

OK. arrays_equals changes reverted.

vnkozlov

Good.

asgibbons · 2024-04-22T22:31:05Z

/integrate

Thanks!

openjdk · 2024-04-22T22:31:15Z

@asgibbons
Your change (at version f7d7f7d) is now ready to be sponsored by a Committer.

eme64

Looks reasonable. Can you apply the indentation issue, please?

eme64 · 2024-04-23T16:03:42Z

src/hotspot/cpu/x86/macroAssembler_x86.hpp

+// * No condition for this *  void ALWAYSINLINE jcxz(Label& L, bool maybe_short = true) { jcc(Assembler::cxz, L, maybe_short); }
+// * No condition for this *  void ALWAYSINLINE jecxz(Label& L, bool maybe_short = true) { jcc(Assembler::cxz, L, maybe_short); }
+
+// Short versions of the above


Suggested change

// * No condition for this * void ALWAYSINLINE jcxz(Label& L, bool maybe_short = true) { jcc(Assembler::cxz, L, maybe_short); }

// * No condition for this * void ALWAYSINLINE jecxz(Label& L, bool maybe_short = true) { jcc(Assembler::cxz, L, maybe_short); }

// Short versions of the above

// * No condition for this * void ALWAYSINLINE jcxz(Label& L, bool maybe_short = true) { jcc(Assembler::cxz, L, maybe_short); }

// * No condition for this * void ALWAYSINLINE jecxz(Label& L, bool maybe_short = true) { jcc(Assembler::cxz, L, maybe_short); }

// Short versions of the above

Everywhere else it is indented, so it would be nice if this kept the style

eme64 · 2024-04-23T16:03:55Z

src/hotspot/cpu/x86/macroAssembler_x86.hpp

+// * No condition for this *  void ALWAYSINLINE jcxz_b(Label& L) { jccb(Assembler::cxz, L); }
+// * No condition for this *  void ALWAYSINLINE jecxz_b(Label& L) { jccb(Assembler::cxz, L); }


Suggested change

// * No condition for this * void ALWAYSINLINE jcxz_b(Label& L) { jccb(Assembler::cxz, L); }

// * No condition for this * void ALWAYSINLINE jecxz_b(Label& L) { jccb(Assembler::cxz, L); }

// * No condition for this * void ALWAYSINLINE jcxz_b(Label& L) { jccb(Assembler::cxz, L); }

// * No condition for this * void ALWAYSINLINE jecxz_b(Label& L) { jccb(Assembler::cxz, L); }

eme64 · 2024-04-23T16:15:43Z

Thanks for the update. I can sponsor as soon as you attempt integration again.

asgibbons · 2024-04-23T16:54:02Z

Thank you. I'm waiting on @sviswa7 review before integrating again

sviswa7 · 2024-04-23T17:55:24Z

src/hotspot/cpu/x86/macroAssembler_x86.cpp

  Assembler::vpcmpeqb(dst, nds, src, vector_len);
 }

+void MacroAssembler::vpcmpeqb(XMMRegister dst, XMMRegister src1, Address src2, int vector_len) {


The assert is missing here:
assert(((dst->encoding() < 16 && src1->encoding() < 16) || VM_Version::supports_avx512vlbw()),"XMM register should be 0-15");

sviswa7 · 2024-04-23T18:01:02Z

src/hotspot/cpu/x86/macroAssembler_x86.hpp

+  // Adding more natural conditional jump instructions
+  void ALWAYSINLINE jo(Label& L, bool maybe_short = true) { jcc(Assembler::overflow, L, maybe_short); }
+  void ALWAYSINLINE jno(Label& L, bool maybe_short = true) { jcc(Assembler::noOverflow, L, maybe_short); }
+  void ALWAYSINLINE js(Label& L, bool maybe_short = true) { jcc(Assembler::positive, L, maybe_short); }


Isn't js -> jump is sign flag is set -> Assembler::negative?
Correspondingly jns, js_b, jns_b should also be corrected.

sviswa7 · 2024-04-23T18:13:38Z

src/hotspot/cpu/x86/assembler_x86.cpp

+  emit_int8(0x66);
+  prefix(dst, reg);
+  emit_int8((unsigned char)0x39);
+  emit_operand(reg, dst, 1);


This should be emit_operand(reg, dst, 0);

sviswa7 · 2024-04-23T18:26:09Z

src/hotspot/cpu/x86/assembler_x86.cpp

+void Assembler::vpcmpeqw(XMMRegister dst, XMMRegister nds, Address src, int vector_len) {
+  assert(vector_len == AVX_128bit ? VM_Version::supports_avx() : VM_Version::supports_avx2(), "");


InstructionMark missing in this instruction which takes Address as operand?

sviswa7 · 2024-04-23T18:28:23Z

src/hotspot/cpu/x86/assembler_x86.cpp

  emit_int16(0x74, (0xC0 | encode));
 }

+void Assembler::vpcmpeqb(XMMRegister dst, XMMRegister src1, Address src2, int vector_len) {


InstructionMark missing in this instruction as well.

asgibbons · 2024-04-23T19:00:28Z

@sviswa7 Thanks for the good catches. Fixed.

sviswa7

Looks good to me.

asgibbons · 2024-04-23T20:20:25Z

Thanks!

/integrate

openjdk · 2024-04-23T20:20:33Z

@asgibbons
Your change (at version f4db9a1) is now ready to be sponsored by a Committer.

sviswa7 · 2024-04-23T23:35:07Z

/sponsor

openjdk · 2024-04-23T23:36:00Z

Going to push as commit 7a89555.
Since your change was applied there have been 32 commits pushed to the master branch:

f60798a: 8329222: java.text.NumberFormat (and subclasses) spec updates
2555166: 8329113: Deprecate -XX:+UseNotificationThread
09b8809: 8327289: Remove unused PrintMethodFlushingStatistics option
9cc163a: 8330178: Clean up non-standard use of /** comments in java.base
88a5dce: 8330805: ARM32 build is broken after JDK-8139457
7157eea: 8327290: Remove unused notproduct option TraceInvocationCounterOverflow
b4cea70: 8330693: Generational ZGC: Simplify ZAddress::finalizable_good and ZAddress::mark_good
412e306: 8329593: Drop adjustments to target parallelism when virtual threads do I/O on files opened for buffered I/O
b07e153: 8330362: G1: Inline offset array element accessor in G1BlockOffsetTable
b6518a5: 8329417: Remove objects with no pointers from relocation bitmap
... and 22 more: https://git.openjdk.org/jdk/compare/936a47d7d68c7305993f68db844dc76006b674b1...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-23T23:36:08Z

@sviswa7 @asgibbons Pushed as commit 7a89555.

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

Add conditional jump aliases; move arrays_equals; add instructions

74d4730

openjdk bot changed the title ~~JDK-8330844 Add aliases for conditional jumps and additional instruction forms for x86~~ 8330844: Add aliases for conditional jumps and additional instruction forms for x86 Apr 22, 2024

openjdk bot added the rfr Pull request is ready for review label Apr 22, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label Apr 22, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Apr 22, 2024

Undo move of arrays_equals

0b95b3a

vnkozlov reviewed Apr 22, 2024

View reviewed changes

Revert changes to arrays_equals

f7d7f7d

vnkozlov approved these changes Apr 22, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 22, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 22, 2024

eme64 approved these changes Apr 23, 2024

View reviewed changes

Comment indentation

fe5f306

openjdk bot removed the sponsor Pull request is ready to be sponsored label Apr 23, 2024

eme64 approved these changes Apr 23, 2024

View reviewed changes

sviswa7 reviewed Apr 23, 2024

View reviewed changes

Review comments

f4db9a1

sviswa7 approved these changes Apr 23, 2024

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 23, 2024

openjdk bot added the integrated Pull request has been integrated label Apr 23, 2024

openjdk bot closed this Apr 23, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 23, 2024

asgibbons deleted the piecemeal branch April 23, 2024 23:41

vnkozlov mentioned this pull request Jun 6, 2024

8333566: Remove unused methods #19550

Closed

3 tasks

		assert((!expand_ary2) \|\| ((expand_ary2) && (UseAVX == 2)),
		"Expansion only implemented for AVX2");

		// * No condition for this * void ALWAYSINLINE jcxz_b(Label& L) { jccb(Assembler::cxz, L); }
		// * No condition for this * void ALWAYSINLINE jecxz_b(Label& L) { jccb(Assembler::cxz, L); }

		void Assembler::vpcmpeqw(XMMRegister dst, XMMRegister nds, Address src, int vector_len) {
		assert(vector_len == AVX_128bit ? VM_Version::supports_avx() : VM_Version::supports_avx2(), "");

8330844: Add aliases for conditional jumps and additional instruction forms for x86 #18893

8330844: Add aliases for conditional jumps and additional instruction forms for x86 #18893

Uh oh!

Conversation

asgibbons commented Apr 22, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Apr 22, 2024

Uh oh!

openjdk bot commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Apr 22, 2024

Uh oh!

asgibbons commented Apr 22, 2024

Uh oh!

openjdk bot commented Apr 22, 2024

Uh oh!

asgibbons commented Apr 22, 2024

Uh oh!

mlbridge bot commented Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

vnkozlov commented Apr 22, 2024

Uh oh!

asgibbons commented Apr 22, 2024

Uh oh!

vnkozlov commented Apr 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asgibbons commented Apr 22, 2024

Uh oh!

vnkozlov commented Apr 22, 2024

Uh oh!

asgibbons commented Apr 22, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

asgibbons commented Apr 22, 2024

Uh oh!

openjdk bot commented Apr 22, 2024

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eme64 commented Apr 23, 2024

Uh oh!

asgibbons commented Apr 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asgibbons commented Apr 23, 2024

asgibbons commented Apr 22, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 22, 2024 •

edited

Loading

mlbridge bot commented Apr 22, 2024 •

edited

Loading