8347459: C2: missing transformation for chain of shifts/multiplications by constants #23728

marc-chevalier · 2025-02-21T15:57:30Z

This collapses double shift lefts by constants in a single constant: (x << con1) << con2 => x << (con1 + con2). Care must be taken in the case con1 + con2 is bigger than the number of bits in the integer type. In this case, we must simplify to 0.

Moreover, the simplification logic of the sign extension trick had to be improved. For instance, we use (x << 16) >> 16 to convert a 32 bits into a 16 bits integer, with sign extension. When storing this into a 16-bit field, this can be simplified into simple x. But in the case where x is itself a left-shift expression, say y << 3, this PR makes the IR looks like (y << 19) >> 16 instead of the old ((y << 3) << 16) >> 16. The former logic didn't handle the case where the left and the right shift have different magnitude. In this PR, I generalize this simplification to cases where the left shift has a larger magnitude than the right shift. This improvement was needed not to miss vectorization opportunities: without the simplification, we have a left shift and a right shift instead of a single left shift, which confuses the type inference.

This also works for multiplications by powers of 2 since they are already translated into shifts.

Thanks,
Marc

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-8347459: C2: missing transformation for chain of shifts/multiplications by constants (Enhancement - P4)

Reviewers

Damon Fenacci (@dafedafe - Committer)
Emanuel Peter (@eme64 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23728

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-02-21T15:58:28Z

👋 Welcome back marc-chevalier! 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 · 2025-02-21T15:58:56Z

@marc-chevalier 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:

8347459: C2: missing transformation for chain of shifts/multiplications by constants

Reviewed-by: dfenacci, epeter

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

c94bc74: 8352595: Regression of JDK-8314999 in IR matching
48fac66: 8347406: [REDO] C1/C2 don't handle allocation failure properly during initialization (RuntimeStub::new_runtime_stub fatal crash)
99c8a6e: 8350463: AArch64: Add vector rearrange support for small lane count vectors
b2da0d3: 8352289: [macos] Review skipped tests in tools/jpackage/macosx/SigningPackage*
ba658a7: 8349522: AArch64: Add backend implementation for new unsigned and saturating vector operations
5625b43: 8350429: runtime/NMT/CheckForProperDetailStackTrace.java should only run for debug JVM
2c60fc5: 8352176: Automate setting up environment for mac signing tests
6e6a39d: 8347321: [ubsan] CGGlyphImages.m:553:30: runtime error: nan is outside the range of representable values of type 'unsigned long'
b84b292: 8352615: [Test] RISC-V: TestVectorizationMultiInvar.java fails on riscv64 without rvv support
a54445f: 8350609: Cleanup unknown unwind opcode (0xB) for windows
... and 10 more: https://git.openjdk.org/jdk/compare/e23e0f85ef0f959a68adda0cff9e721ba2173ffc...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 (@jaskarth, @dafedafe, @eme64) 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 · 2025-02-21T15:59:23Z

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

mlbridge · 2025-02-21T16:02:26Z

Webrevs

jaskarth

This is a nice improvement! I'm glad to see that the limitation of Store/Shift folding requiring both shifts to have the same constant is being fixed. I've left some comments here.

I also noticed that in LShiftINode::Ideal, there is a transform that mentions not breaking i2s and i2b patterns. It might be worth looking separately to see if this condition can be relaxed because of the change to StoreNode::Ideal_sign_extended_input.

src/hotspot/share/opto/mulnode.cpp

src/hotspot/share/opto/memnode.cpp

src/hotspot/share/opto/mulnode.cpp

dean-long · 2025-02-21T23:05:26Z

Care must be taken in the case con1 + con2 is bigger than the number of bits in the integer type. In this case, we must simplify to 0.

So 1 << 33 and 1 << 30 << 3 are still treated differently, according to the JVM spec?

marc-chevalier · 2025-02-24T10:42:25Z

@dean-long Yes! 1 << 33 was already, and is still, transformed into 1 << 1, while 1 << 30 << 3 is NOT transformed into 1 << 33 but directly into 0. The second part is exhibited by this test:

    @Test
    @IR(failOn = {IRNode.LSHIFT})
    // Checks (x << 31) << 1 => 0
    public int testDoubleShift3(int x) {
        return (x << 31) << 1;
    }

(and a few similar other). I didn't add a test for the simple 1 << 33 since my code doesn't kick in unless there are 2 shifts, so nothing should have changed here. I can add such a test if you think it's needed.

dean-long · 2025-02-24T23:46:51Z

I didn't add a test for the simple 1 << 33 since my code doesn't kick in unless there are 2 shifts, so nothing should have changed here. I can add such a test if you think it's needed.

I don't know if it's needed (we might have that covered in other tests), but adding now seems like a good idea for completeness.

… << 30) << 3 and x << 33

dafedafe

Cool improvement @marc-chevalier! Thanks a lot!

test/hotspot/jtreg/compiler/c2/irTests/LShiftINodeIdealizationTests.java

dafedafe

I just left one more small comment. Otherwise it looks good to me. Thanks @marc-chevalier!

test/hotspot/jtreg/compiler/c2/irTests/LShiftINodeIdealizationTests.java

…ifts-multiplications-by-constants

eme64

@marc-chevalier This is good work, thanks for working on this :)
I have a first batch of comments and suggestions.

eme64 · 2025-03-07T12:22:50Z

src/hotspot/share/opto/memnode.cpp

-          set_req_X(MemNode::ValueIn, shl->in(1), phase);
-          return this;
+// If (conIL > conIR) we are inventing 0 lower bits, and throwing
+// away upper bits, but we are not introducing garbage bits by above.


What do you mean with by above?

I mean from the upper bits, from the left if written like decimal numbers. Rephrasing that.

eme64 · 2025-03-07T12:29:10Z

src/hotspot/share/opto/memnode.cpp

+// This case happens when the store source was itself a left shift, that gets merged
+// into the inner left shift of the sign-extension.


Hmm, above you were talking about a left and a right shift. But now you seem to be talking about some "source" left shift and an "inner" left shift. It's a bit confusing. Are you talking about this case?
(StoreB ... (RShiftI _ (LShiftI _ (LShiftI _ valIn conIL1 ) conIL2 ) conIR) )
Where the two left shifts are already combined by Ideal earlier?

Yes indeed. You seem confused about the term "source". I'm happy to change it, but I can't see a better word: a store (or move, or assignment) has a source and a destination, that are often resp. the left-hand side and the right-hand side of the expression/statement/instruction. I'm rephrasing that.

eme64 · 2025-03-07T12:34:36Z

src/hotspot/share/opto/memnode.cpp

+// +------------------------+---------+
+//  31                     8 7        0
+// v[0..7] is meaningful, but v[8..31] is not. In this case, num_rejected_bits == 24
+// If we do the shift left then right by 24 bits, we get:


It could be nice if you explicitly denoted the 3 cases, maybe even using some indentation for emphasis:

Case 1: conIL == conIR Case 2: conIL > conIR Case 3: conIL < conIR

It could also be nice to say what you are trying to show / prove here.

eme64 · 2025-03-07T12:35:28Z

src/hotspot/share/opto/memnode.cpp

+// +------------------+---------+-----+
+// |     sign bit     | v[0..5] |  0  |
+// +------------------+---------+-----+
+//  31               8 7       2 1   0


Can you make a statement if this is ok or not?

eme64 · 2025-03-07T12:37:38Z

src/hotspot/share/opto/memnode.cpp

+// |     sign bit     | v[0..5] |  0  |
+// +------------------+---------+-----+
+//  31              10 9       4 3   0
+//


Do we also have a case where conIL < conIR? What happens then?

src/hotspot/share/opto/memnode.cpp

src/hotspot/share/opto/mulnode.cpp

test/hotspot/jtreg/compiler/c2/irTests/LShiftLNodeIdealizationTests.java

src/hotspot/share/opto/memnode.cpp

marc-chevalier · 2025-03-20T15:48:43Z

Fixed all the comments. You can do the next iteration.

eme64

@marc-chevalier Nice work. Thanks for all the updates, and extra time spent on the proof!

dafedafe

Thanks for reworking on the explanation etc. @marc-chevalier!
Still looks ok to me.

…ifts-multiplications-by-constants

marc-chevalier · 2025-03-25T07:56:50Z

/integrate

I've merged master in it, ran more tests. Seems all good.
Thanks for the many reviews!

openjdk · 2025-03-25T07:58:15Z

@marc-chevalier
Your change (at version cd0b0c0) is now ready to be sponsored by a Committer.

TobiHartmann · 2025-03-25T10:15:22Z

/sponsor

eme64 · 2025-03-25T10:15:28Z

/sponsor

openjdk · 2025-03-25T10:15:58Z

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

3d3b782: 8352607: RISC-V: use cmove in min/max when Zicond is supported
9f582e5: 8320997: RISC-V: C2 ReverseV
6879c44: 8351405: G1: Collection set early pruning causes suboptimal region selection
aee4d69: 8348829: Remove ObjectMonitor perf counters
f9bcef4: 8351627: C2 AArch64 ROR/ROL: assert((1 << ((T>>1)+3)) > shift) failed: Invalid Shift value
17dc30c: 8352414: JFR: JavaMonitorDeflateEvent crashes when deflated monitor object is dead
6bc4803: 8351277: Remove pipewire from AIX build
c94bc74: 8352595: Regression of JDK-8314999 in IR matching
48fac66: 8347406: [REDO] C1/C2 don't handle allocation failure properly during initialization (RuntimeStub::new_runtime_stub fatal crash)
99c8a6e: 8350463: AArch64: Add vector rearrange support for small lane count vectors
... and 17 more: https://git.openjdk.org/jdk/compare/e23e0f85ef0f959a68adda0cff9e721ba2173ffc...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-03-25T10:16:11Z

@TobiHartmann @marc-chevalier Pushed as commit bdcac98.

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

openjdk · 2025-03-25T10:16:24Z

@eme64 The command sponsor can only be used in open pull requests.

marc-chevalier added 8 commits February 19, 2025 12:46

collapse lshift with constants

ee8eea1

more tests

1b1a84d

register for igvn

cd8a90f

clang-format

40e4f9a

fix type bug

4b40c66

format

7102dce

actually return a new node

07a499c

improve simplification of double shifts in stores

bb867b6

openjdk bot added the rfr Pull request is ready for review label Feb 21, 2025

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Feb 21, 2025

jaskarth reviewed Feb 21, 2025

View reviewed changes

marc-chevalier added 5 commits February 25, 2025 13:39

Add test suggested by @dean-long exhibiting the difference between (x…

932a32d

… << 30) << 3 and x << 33

rename

7a1bb1b

Remove useless local, with especially helpful name

7e33134

Fix style in the few lines I haven't touched yet

84d18df

comment

c792090

dafedafe reviewed Feb 26, 2025

View reviewed changes

test/hotspot/jtreg/compiler/c2/irTests/LShiftINodeIdealizationTests.java Show resolved Hide resolved

+ comment on why not zerocon

b8c3d74

dafedafe approved these changes Mar 7, 2025

View reviewed changes

test/hotspot/jtreg/compiler/c2/irTests/LShiftINodeIdealizationTests.java Outdated Show resolved Hide resolved

marc-chevalier added 2 commits March 7, 2025 14:00

Merge branch 'master' into fix/missing-transformation-for-chain-of-sh…

e71cb56

…ifts-multiplications-by-constants

use a random number in testing

8e93a12

eme64 suggested changes Mar 7, 2025

View reviewed changes

eme64 reviewed Mar 7, 2025

View reviewed changes

test/hotspot/jtreg/compiler/c2/irTests/LShiftLNodeIdealizationTests.java Show resolved Hide resolved

marc-chevalier added 9 commits March 11, 2025 10:33

nit

230af28

char -> byte

abe137a

rephrase

7421ea5

rephrased corner case

ad563ba

s

be2f7b7

correct

f244e17

rephrase

a96a219

order

5afbe37

more checks

e3ecf35

eme64 reviewed Mar 20, 2025

View reviewed changes

src/hotspot/share/opto/memnode.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/memnode.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/memnode.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/memnode.cpp Outdated Show resolved Hide resolved

Rephrase comment

124d938

eme64 approved these changes Mar 21, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 21, 2025

dafedafe approved these changes Mar 24, 2025

View reviewed changes

Merge branch 'master' into fix/missing-transformation-for-chain-of-sh…

cd0b0c0

…ifts-multiplications-by-constants

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 25, 2025

openjdk bot added the integrated Pull request has been integrated label Mar 25, 2025

openjdk bot closed this Mar 25, 2025

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 Mar 25, 2025

marc-chevalier mentioned this pull request Apr 22, 2025

8320909: C2: Adapt IGVN's enqueuing logic to match idealization of AndNode with LShift operand #24792

Closed

3 tasks

		// This case happens when the store source was itself a left shift, that gets merged
		// into the inner left shift of the sign-extension.

8347459: C2: missing transformation for chain of shifts/multiplications by constants #23728

8347459: C2: missing transformation for chain of shifts/multiplications by constants #23728

Uh oh!

Conversation

marc-chevalier commented Feb 21, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 21, 2025

Uh oh!

openjdk bot commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Feb 21, 2025

Uh oh!

mlbridge bot commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

jaskarth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dean-long commented Feb 21, 2025

Uh oh!

marc-chevalier commented Feb 24, 2025

Uh oh!

dean-long commented Feb 24, 2025

Uh oh!

dafedafe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dafedafe left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marc-chevalier commented Mar 20, 2025

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

dafedafe left a comment

Choose a reason for hiding this comment

marc-chevalier commented Feb 21, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 21, 2025 •

edited

Loading

mlbridge bot commented Feb 21, 2025 •

edited

Loading