8297384: Add IR tests for existing idealizations of arithmetic nodes

[CptGit]

I noticed some idealizations have no associated IR tests so I included for them.

---

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-8297384: Add IR tests for existing idealizations of arithmetic nodes

Reviewers

• Christian Hagedorn (@chhagedorn - Reviewer) ⚠️ Review applies to c0931397

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11049

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

Using diff file

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

[bridgekeeper]

👋 Welcome back CptGit! 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]

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

[y1yang0]

Hi, do you need a JBS issue for this?

[CptGit]

Hi, do you need a JBS issue for this?

Hi, I do not have an OpenJDK account, so can you open one issue for me? Thanks!

[y1yang0]

Okay, I filed JDK-8297384 Add IR tests for existing idealizations of arithmetic nodes for this patch.

[mlbridge]

Webrevs

• 03: Full - Incremental (02c2d5e9)
• 02: Full - Incremental (bb92a752)
• 01: Full - Incremental (c0931397)
• 00: Full (6930c908)

[CptGit]

Okay, I filed JDK-8297384 Add IR tests for existing idealizations of arithmetic nodes for this patch.

Thanks a lot! I included the issue id in this PR. Could you please help review the PR if you get a chance, thanks!

[chhagedorn]

Nice additional tests! They look good.

[openjdk]

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

8297384: Add IR tests for existing idealizations of arithmetic nodes

Reviewed-by: chagedorn

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

• df6cf1e: 8296886: Fix various include sort order issues
• 5e196b4: 8297476: Increase InlineSmallCode default from 1000 to 2500 for RISC-V
• 070a84c: 8297192: Warning generating API docs for javax.management.MBeanServer: overridden methods do not document exception type
• 3c4d520: 8296671: [JFR] jdk.ContainerConfiguration event should include host total memory
• 8b73970: 8296208: AArch64: Enable SHA512 intrinsic by default on supported hardware
• f4cf758: 8297491: Loom: Stack chunks allocation code uses TLABs even when TLABs are disabled
• 5e5b774: 8296768: Use different explicit claim marks for CLDs in Parallel and Serial GC
• 9c77e41: 8297445: PPC64: Represent Registers as values
• 2f8a5c2: 8297480: GetPrimitiveArrayCritical in imageioJPEG misses result - NULL check
• abeddab: 6528710: sRGB-ColorSpace to sRGB-ColorSpace Conversion
• ... and 27 more: https://git.openjdk.org/jdk/compare/ccc6e169188019dab008df5ffb11b4279e14f8fc...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 (@chhagedorn) 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).

[CptGit]

Nice additional tests! They look good.

Thank you for reviewing. I noticed RotateLeftNodeIntIdealizationTests.java and RotateLeftNodeLongIdealizationTests.java failed on x86. Does this matter should we make sure they pass?

[chhagedorn]

We should make sure to make them pass on x86 as well such that GHA is clean after the integration of this PR. I've had a quick look on the match rules and it seems that we only use RotateLeft/Right nodes on x86_64, aarch64 and riscv. We could restrict these tests to only run on these architectures with @requires os.arch == "x86_64" | os.arch == "aarch64" | os.arch == "riscv64".

[CptGit]

We should make sure to make them pass on x86 as well such that GHA is clean after the integration of this PR. I've had a quick look on the match rules and it seems that we only use RotateLeft/Right nodes on x86_64, aarch64 and riscv. We could restrict these tests to only run on these architectures with @requires os.arch == "x86_64" | os.arch == "aarch64" | os.arch == "riscv64".

Thanks for your help! I have added the restrictions and the GHA should be clean now. I have a question though: where did you see the architectures where RotateLeft/Right nodes are used? so that I can know when the the same case happens again. Thanks.

[CptGit]

/integrate

[openjdk]

@CptGit
Your change (at version 02c2d5e) is now ready to be sponsored by a Committer.

[chhagedorn]

We should make sure to make them pass on x86 as well such that GHA is clean after the integration of this PR. I've had a quick look on the match rules and it seems that we only use RotateLeft/Right nodes on x86_64, aarch64 and riscv. We could restrict these tests to only run on these architectures with @requires os.arch == "x86_64" | os.arch == "aarch64" | os.arch == "riscv64".

Thanks for your help! I have added the restrictions and the GHA should be clean now. I have a question though: where did you see the architectures where RotateLeft/Right nodes are used? so that I can know when the the same case happens again. Thanks.

Since RotateLeft/Right are not macro nodes or other intermediate nodes like Opaque* which go away during optimizations, we need to find a match rule for them in order to build the mach graph. I therefore just searched for match\(.*Rotate and only found rules in aarch64.ad, x86_64.ad and riscv_b.ad (in x86.ad, we only find rules for the vector rotate nodes Rotate.*V).

[chhagedorn]

/sponsor

[openjdk]

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

• cfe5a37: 8297556: Parse::check_interpreter_type fails with assert "must constrain OSR typestate"
• 74d3bac: 8297417: Poly1305IntrinsicFuzzTest fails with tag mismatch exception
• 0ed8b33: 8297385: Remove duplicated null typos in javadoc
• 1fb9dad: 8296419: [REDO] JDK-8295319: pending_cards_at_gc_start doesn't include cards in thread buffers
• 2f47f83: 8297569: URLPermission constructor throws IllegalArgumentException: Invalid characters in hostname after JDK-8294378
• 87d1097: 8297530: java.lang.IllegalArgumentException: Negative length on strings concatenation
• 390e69a: 8297150: Add a @sealedGraph tag to Reference
• 8effaa8: 8223783: sun/net/www/http/HttpClient/MultiThreadTest.java sometimes detect threads+1 connections
• ab1f9ff: 8051627: Invariants about java.net.URI resolve and relativize are wrong
• df6cf1e: 8296886: Fix various include sort order issues
• ... and 36 more: https://git.openjdk.org/jdk/compare/ccc6e169188019dab008df5ffb11b4279e14f8fc...master

Your commit was automatically rebased without conflicts.

[openjdk]

@chhagedorn @CptGit Pushed as commit fd910f7.

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