8324790: ifnode::fold_compares_helper cleanup

[caojoshua]

I hope my assumptions in filtered_int_type are correct here:

• we assert that if_proj is an IfTrue or IfFalse, so it is safe to assume if_proj->_in is an IfNode
• the 1'th input of a CmpNode is a BoolNode
• Tthe 1'th input of an IfNode is not always a BoolNode, it can be a constant. We need to leave this check in.

We also remove a some of the if-checks in compare_folds_cleanup which seem unnecessary.

Passes tier1 locally.

---

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-8324790: ifnode::fold_compares_helper cleanup (Enhancement - P5)

Reviewers

• Christian Hagedorn (@chhagedorn - Reviewer)
• Emanuel Peter (@eme64 - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17601

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

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

@caojoshua 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]

Webrevs

• 02: Full - Incremental (341b5006)
• 01: Full - Incremental (19e1fa81)
• 00: Full (eb6f4574)

[eme64]

I have not yet looked at your code. But I'm not sure your assumptions are all correct:

we assert that if_proj is an IfTrue or IfFalse, so it is safe to assume if_proj->_in is an IfNode

Generally, during IGVN, ctrl inputs could always be TOP, if we are in a dead-code place. Are you sure there is always an IfNode above the projections? Maybe that is true in this specific call chain?

the 1'th input of a CmpNode is a BoolNode

Do you mean the 1st input of BoolNode is a CmpNode? I think we may also fold CmpNodes to constants, or TOP. Is that somehow excluded in your cases?

The problem with testing of this: getting TOP inputs is kinda rare. But you can make it more likely with StressIGVN.

[caojoshua]

I have not yet looked at your code. But I'm not sure your assumptions are all correct:

we assert that if_proj is an IfTrue or IfFalse, so it is safe to assume if_proj->_in is an IfNode

Generally, during IGVN, ctrl inputs could always be TOP, if we are in a dead-code place. Are you sure there is always an IfNode above the projections? Maybe that is true in this specific call chain?

the 1'th input of a CmpNode is a BoolNode

Do you mean the 1st input of BoolNode is a CmpNode? I think we may also fold CmpNodes to constants, or TOP. Is that somehow excluded in your cases?

The problem with testing of this: getting TOP inputs is kinda rare. But you can make it more likely with StressIGVN.

I removed all my changes in filtered_int_type(). There are some other callers which may have some unexpected constants or TOPs.

My main motivation anyway was in fold_compares_helper(), and I think that cleanup is still valid.

[eme64]

I think this generally looks reasonable. I'm running some testing now...

/reviewers 2

[openjdk]

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

[chhagedorn]

The current patch looks good to me.

[eme64]

Thanks for the updates, looks good now :)

[openjdk]

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

8324790: ifnode::fold_compares_helper cleanup

Reviewed-by: chagedorn, 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 68 new commits pushed to the master branch:

• b8fc418: 8326525: com/sun/tools/attach/BasicTests.java does not verify AgentLoadException case
• d9aa1de: 8318605: Enable parallelism in vmTestbase/nsk/stress/stack tests
• bbfda65: 8326897: (fs) The utility TestUtil.supportsLinks is wrongly used to check for hard link support
• db0e2b8: 8326944: (ch) Minor typo in the ScatteringByteChannel.read(ByteBuffer[] dsts,int offset,int length) javadoc
• 8f6edd8: 8326975: Parallel: Remove redundant PSOldGen::is_allocated
• 4302900: 8319673: Few security tests ignore VM flags
• e772e78: 8326948: Force English locale for timeout formatting
• d9ef16d: 8326140: src/jdk.accessibility/windows/native/libjavaaccessbridge/AccessBridgeJavaEntryPoints.cpp ReleaseStringChars might be missing in early returns
• 998d0ba: 8324799: Use correct extension for C++ test headers
• 0735c8a: 8318302: ThreadCountLimit.java failed with "Native memory allocation (mprotect) failed to protect 16384 bytes for memory to guard stack pages"
• ... and 58 more: https://git.openjdk.org/jdk/compare/63f6a563a3987d74ef673718d5209cc7c469751c...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, @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).

[caojoshua]

/integrate

[caojoshua]

@eme64 could you sponsor?

[openjdk]

@caojoshua
Your change (at version 341b500) is now ready to be sponsored by a Committer.

[eme64]

/sponsor

[openjdk]

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

• 0d35450: 8327040: Problemlist ActionListenerCalledTwiceTest.java test failing in macos14
• 8d6f784: 8327056: Remove unused static char array in JvmtiAgentList::lookup
• 43af120: 8326959: Improve JVMCI option help
• 742c776: 8322743: C2: prevent lock region elimination in OSR compilation
• d29cefb: 8326838: JFR: Native mirror events
• b8fc418: 8326525: com/sun/tools/attach/BasicTests.java does not verify AgentLoadException case
• d9aa1de: 8318605: Enable parallelism in vmTestbase/nsk/stress/stack tests
• bbfda65: 8326897: (fs) The utility TestUtil.supportsLinks is wrongly used to check for hard link support
• db0e2b8: 8326944: (ch) Minor typo in the ScatteringByteChannel.read(ByteBuffer[] dsts,int offset,int length) javadoc
• 8f6edd8: 8326975: Parallel: Remove redundant PSOldGen::is_allocated
• ... and 63 more: https://git.openjdk.org/jdk/compare/63f6a563a3987d74ef673718d5209cc7c469751c...master

Your commit was automatically rebased without conflicts.

[openjdk]

@eme64 @caojoshua Pushed as commit 12404a5.

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