8303238: Create generalizations for existing LShift ideal transforms #12734

jaskarth · 2023-02-23T20:28:31Z

Hello,
I would like to generalize two ideal transforms for bitwise shifts. Left shift nodes perform the transformations (x >> C1) << C2 => x & (-1 << C2) and ((x >> C1) & Y) << C2 => x & (Y << C2), but only when the case where C1 == C2. However, it is possible to use both of these rules to improve cases where the constants aren't equal, by removing one of the shifts and replacing it with a bitwise and. This transformation is profitable because typically more bitwise ands can be dispatched per cycle than bit shifts. In addition, the strength reduction from a shift to a bitwise and can allow more profitable transformations to occur. These patterns are found throughout the JDK, mainly around strings and OW2 ASM. I've attached some profiling results from my (Zen 2) machine below:

                                                 Baseline                           Patch              Improvement
Benchmark                            Mode  Cnt    Score     Error  Units      Score    Error  Units
LShiftNodeIdealize.testRgbaToAbgr    avgt   15    63.287 ±  1.770  ns/op  /  54.199 ±  1.408  ns/op     + 14.36%
LShiftNodeIdealize.testShiftAndInt   avgt   15   874.564 ± 15.334  ns/op  / 538.408 ± 11.768  ns/op     + 38.44%
LShiftNodeIdealize.testShiftAndLong  avgt   15  1017.466 ± 29.010  ns/op  / 701.356 ± 18.258  ns/op     + 31.07%
LShiftNodeIdealize.testShiftInt      avgt   15   663.865 ± 14.226  ns/op  / 533.588 ±  9.949  ns/op     + 19.63%
LShiftNodeIdealize.testShiftInt2     avgt   15   658.976 ± 32.856  ns/op  / 649.871 ± 10.598  ns/op     +  1.38%
LShiftNodeIdealize.testShiftLong     avgt   15   815.540 ± 14.721  ns/op  / 689.270 ± 14.028  ns/op     + 15.48%
LShiftNodeIdealize.testShiftLong2    avgt   15   817.936 ± 23.573  ns/op  / 810.185 ± 14.983  ns/op     +  0.95%

In addition, in the process of making this PR I've found a missing ideal transform for RShiftLNode, so right shifts of large numbers (such as x >> 65) are not properly folded down, like how they are RShiftINode and URShiftLNode. I'll address this in a future RFR.

Testing: GHA, tier1 local, and performance testing

Thanks,
Jasmine K

Progress

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

Issue

JDK-8303238: Create generalizations for existing LShift ideal transforms

Reviewers

Claes Redestad (@cl4es - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to bd161561

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12734

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

Using diff file

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

bridgekeeper · 2023-02-23T20:30:11Z

👋 Welcome back SuperCoder7979! 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 · 2023-02-23T20:32:21Z

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

TobiHartmann · 2023-02-27T11:33:58Z

Please link to JDK-8303238.

mlbridge · 2023-02-27T15:26:58Z

Webrevs

cl4es

Very nice overall!

Some superficial comments inline.

cl4es · 2023-02-27T16:24:00Z

src/hotspot/share/opto/mulnode.cpp

-      add1->in(2) == in(2) )
-    // Convert to "(x & -(1<<c0))"
-    return new AndINode(add1->in(1),phase->intcon( -(1<<con)));
+  // Check for "(x >> C1) << C2" which just masks off low bits


The "which just masks off the low bits" comments should move to the C1 == C2 special case. Same below and for LShiftLNode.

cl4es · 2023-02-27T16:34:27Z

test/micro/org/openjdk/bench/vm/compiler/LShiftNodeIdealize.java

+    @State(Scope.Benchmark)
+    public static class BenchState {
+        int[] ints;
+        Random random = new Random();


A hard-coded or parameterized seed is preferred for microbenchmarking to reduce noise from different data distributions in back-to-back runs.

jaskarth · 2023-03-07T03:43:18Z

Thanks for the comments! I have updated the code.

TobiHartmann

The change looks good to me and internal testing passes. Thanks for the contribution!

This transformation is profitable because typically more bitwise ands can be dispatched per cycle than bit shifts

Are we sure that this is profitable on all architectures?

Another review would be good.

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

openjdk · 2023-03-09T07:52:11Z

⚠️ @SuperCoder7979 the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout left-shift-ideal-generalization
$ git commit --author='Preferred Full Name <you@example.com>' --allow-empty -m 'Update full name'
$ git push

openjdk · 2023-03-09T07:52:13Z

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

8303238: Create generalizations for existing LShift ideal transforms

Reviewed-by: redestad, thartmann

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

f2a36b4: 8302666: Replace CHM with VarHandle in ForeachOrderedTask
a32ee5d: 8303949: gcc10 warning Linux ppc64le - note: the layout of aggregates containing vectors with 8-byte alignment has changed in GCC 5
01312a0: 8300821: UB: Applying non-zero offset to non-null pointer 0xfffffffffffffffe produced null pointer
6d30bbe: 8303001: Add test for re-entrant upcalls
de9f3b6: 8296400: pointCrlIssuers might be null in DistributionPointFetcher::verifyURL
b9951dd: 8303820: Simplify type metadata
75d6306: 8303349: Simplify link format for generic types in index pages
b1d89f3: 8294971: jdk.jlink jdk.tools.jimage.JImageTask is using ASM to verify classes
0f26d09: 8303822: gtestMain should give more helpful output
e26cc52: 8303624: The java.lang.Thread.FieldHolder can be null for JNI attaching threads
... and 237 more: https://git.openjdk.org/jdk/compare/db483a38a815f85bd9668749674b5f0f6e4b27b4...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 (@cl4es, @TobiHartmann) 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).

TobiHartmann · 2023-03-09T07:52:35Z

/reviewers 2

openjdk · 2023-03-09T07:53:01Z

@TobiHartmann
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

jaskarth · 2023-03-09T22:51:06Z

Hi, thanks for the review! I have fixed the mistype in the comments, and have updated the bug headers. As for profitability, I looked through Agner Fog's instruction tables and uops.info and found that for all x86 microarchitectures listed, bitwise and instructions are as good as or faster than bit shift instructions, in terms of how many can be dispatched per cycle. I think even in the cases where the dispatches are the same the transformation can be profitable as the two instructions typically utilize different ports of the processor backend (as seen in uops.info), leading to more thorough utilization of the processor's resources.

For different architectures, I wasn't able to readily find resources on instruction latency such as Agner for x86, but I was able to find LLVM's scheduler models for aarch64, ppc, and risc-v. These all seemed to be similar to x86- where the bitwise and instruction is as good as or better than the shift instructions, while also taking up different processor resources. In addition, due to the constant folding opportunities offered by this change I think it should be applicable, but a review from people familiar with different architectures would be helpful. Hope this clarifies!

cl4es · 2023-03-09T23:18:55Z

Can't say I'm very familiar with aarch64 (yet) but on my Mac M1 (osx-aarch64) I see similar improvements:

                                                Baseline                           Patch          Improvement
Benchmark                            Mode  Cnt    Score    Error  Units      Score   Error  Units
LShiftNodeIdealize.testShiftAndInt   avgt   15  601.106 ± 18,668  ns/op  / 432.041 ± 3.912  ns/op  +39.13%
LShiftNodeIdealize.testShiftAndLong  avgt   15  588.143 ±  2.281  ns/op  / 422.035 ± 5.384  ns/op  +39.36%

jaskarth · 2023-03-10T00:50:20Z

Nice! Glad to see the change has an impact there :)

TobiHartmann · 2023-03-10T07:56:49Z

Thanks for updating the comments and the additional details. Looks good to me!

jaskarth · 2023-03-10T16:08:09Z

Thanks for the reviews!

jaskarth · 2023-03-10T16:08:18Z

/integrate

openjdk · 2023-03-10T16:09:05Z

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

cl4es · 2023-03-10T16:20:08Z

I've started an internal testing run (tier 1-3) and will report any issues or sponsor depending on the results.

The test that's failing in GHA is an unrelated bug that was supposedly fixed yesterday: https://bugs.openjdk.org/browse/JDK-8303105

cl4es · 2023-03-13T11:09:11Z

Testing looks good

/sponsor

openjdk · 2023-03-13T11:10:54Z

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

805a4e6: 8303883: Confusing parameter name in G1UpdateRemSetTrackingBeforeRebuild::distribute_marked_bytes
25e7ac2: 8294966: Convert jdk.jartool/sun.tools.jar.FingerPrint to use the ClassFile API to parse JAR entries
3018b47: 8303969: Limit printed failures within an object during G1 heap verification
b575e54: 8303963: Replace various encodings of UINT/SIZE_MAX in gc code
c183fce: 8300926: Several startup regressions ~6-70% in 21-b6 all platforms
31e1e39: 8303646: [JVMCI] Add possibility to lookup ResolvedJavaType from jclass.
1148a65: 8303678: [JVMCI] Add possibility to convert object JavaConstant to jobject.
d20bde2: 8294715: Add IR checks to the reduction vectorization tests
c313e1a: 8303922: build-test-lib target is broken
fbc76c2: 8304017: ProblemList com/sun/jdi/InvokeHangTest.java on windows-x64 in vthread mode
... and 260 more: https://git.openjdk.org/jdk/compare/db483a38a815f85bd9668749674b5f0f6e4b27b4...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-13T11:11:19Z

@cl4es @SuperCoder7979 Pushed as commit 8e41bf2.

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

bulasevich · 2023-03-15T07:07:27Z

@SuperCoder7979 I see a new issue, it must be related to this change. Can you take a look? https://bugs.openjdk.org/browse/JDK-8304230

jaskarth · 2023-03-15T12:37:11Z

Hi, and apologies- I'll address this ASAP. Thanks for the heads up.

Create generalizations for existing LShift ideal transforms

a1baa1d

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Feb 23, 2023

jaskarth changed the title ~~Create generalizations for existing LShift ideal transforms~~ 8303238: Create generalizations for existing LShift ideal transforms Feb 27, 2023

openjdk bot added the rfr Pull request is ready for review label Feb 27, 2023

cl4es suggested changes Mar 6, 2023

View reviewed changes

Comments from code review

bd16156

TobiHartmann approved these changes Mar 9, 2023

View reviewed changes

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

openjdk bot added the ready Pull request is ready to be integrated label Mar 9, 2023

openjdk bot removed the ready Pull request is ready to be integrated label Mar 9, 2023

Update comments in IR tests

9a6ff3c

Update full name

c4a5d23

cl4es approved these changes Mar 10, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 10, 2023

openjdk bot added the sponsor Pull request is ready to be sponsored label Mar 10, 2023

openjdk bot added the integrated Pull request has been integrated label Mar 13, 2023

openjdk bot closed this Mar 13, 2023

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 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8303238: Create generalizations for existing LShift ideal transforms #12734

8303238: Create generalizations for existing LShift ideal transforms #12734

jaskarth commented Feb 23, 2023 •

edited by openjdk bot

bridgekeeper bot commented Feb 23, 2023

openjdk bot commented Feb 23, 2023

TobiHartmann commented Feb 27, 2023

mlbridge bot commented Feb 27, 2023 •

edited

cl4es left a comment

cl4es Feb 27, 2023

cl4es Feb 27, 2023

jaskarth commented Mar 7, 2023

TobiHartmann left a comment

openjdk bot commented Mar 9, 2023

openjdk bot commented Mar 9, 2023 •

edited

TobiHartmann commented Mar 9, 2023

openjdk bot commented Mar 9, 2023

jaskarth commented Mar 9, 2023

cl4es commented Mar 9, 2023

jaskarth commented Mar 10, 2023

TobiHartmann commented Mar 10, 2023

jaskarth commented Mar 10, 2023

jaskarth commented Mar 10, 2023

openjdk bot commented Mar 10, 2023

cl4es commented Mar 10, 2023

cl4es commented Mar 13, 2023

openjdk bot commented Mar 13, 2023

openjdk bot commented Mar 13, 2023

bulasevich commented Mar 15, 2023

jaskarth commented Mar 15, 2023

8303238: Create generalizations for existing LShift ideal transforms #12734

8303238: Create generalizations for existing LShift ideal transforms #12734

Conversation

jaskarth commented Feb 23, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 23, 2023

openjdk bot commented Feb 23, 2023

TobiHartmann commented Feb 27, 2023

mlbridge bot commented Feb 27, 2023 • edited

Webrevs

cl4es left a comment

Choose a reason for hiding this comment

cl4es Feb 27, 2023

Choose a reason for hiding this comment

cl4es Feb 27, 2023

Choose a reason for hiding this comment

jaskarth commented Mar 7, 2023

TobiHartmann left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 9, 2023

openjdk bot commented Mar 9, 2023 • edited

TobiHartmann commented Mar 9, 2023

openjdk bot commented Mar 9, 2023

jaskarth commented Mar 9, 2023

cl4es commented Mar 9, 2023

jaskarth commented Mar 10, 2023

TobiHartmann commented Mar 10, 2023

jaskarth commented Mar 10, 2023

jaskarth commented Mar 10, 2023

openjdk bot commented Mar 10, 2023

cl4es commented Mar 10, 2023

cl4es commented Mar 13, 2023

openjdk bot commented Mar 13, 2023

openjdk bot commented Mar 13, 2023

bulasevich commented Mar 15, 2023

jaskarth commented Mar 15, 2023

jaskarth commented Feb 23, 2023 •

edited by openjdk bot

mlbridge bot commented Feb 27, 2023 •

edited

openjdk bot commented Mar 9, 2023 •

edited