8334647: C2: CastII added by PhaseIdealLoop::add_template_assertion_predicate() should have control

[rwestrel]

PhaseIdealLoop::add_template_assertion_predicate() creates a new CastII but doesn't set its control. I think it's good practice to always set the control of a CastII when it narrows the type of its input (it has to be control dependent on some condition that makes the narrowing possible). This is also motivated by https://bugs.openjdk.org/browse/JDK-8275202 where a node that becomes top is used as an indication that control flow where the node changes type can constant fold. This only works if control is set correctly.

---

Progress

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

Issue

• JDK-8334647: C2: CastII added by PhaseIdealLoop::add_template_assertion_predicate() should have control (Bug - P4)

Reviewers

• Christian Hagedorn (@chhagedorn - Reviewer)
• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19808

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back roland! 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[bot]]

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

8334647: C2: CastII added by PhaseIdealLoop::add_template_assertion_predicate() should have control

Reviewed-by: chagedorn, kvn

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

• 34ee06f51122e8afb875cc3b75f513912272fd9b: 8332850: javac crashes if container for repeatable annotation is not found
• 00b53481064ed1ca914008d06ce5b530ca7ffe16: 8337192: [BACKOUT] JDK-8336098 G1: Refactor G1RebuildRSAndScrubTask
• 3baff2af6a30cc6cb2e0d4391db1cf7e61c33f64: 8335393: C2: assert(!had_error) failed: bad dominance
• 8081f8702a89c4895302377be62da22fc34f2c74: 8334342: Add MergeStore JMH benchmarks
• 9d8791864ec48f3321707d7f7805cd3618fc3b51: 8335191: RISC-V: verify perf of chacha20
• 6e228ce382d1fad6cba0d0df8a507e6bd32a9a4a: 8336254: Virtual thread implementation + test updates
• d3e51daf7331b84b4e78f7f10360848d7c549c1a: 8334085: Test crash: assert(thread->held_monitor_count() == 0) failed: Must be
• 0898ab7f7496689e5de52a5dc4530ca21def1fca: 8336741: Optimize LocalTime.toString with StringBuilder.repeat
• 24f67d0254c08a668d24f28ec0fa768ef10feed5: 8334232: Optimize C1 classes layout
• 021c2c36ac243009c71147072d405636cab0b12c: 8337067: Test runtime/classFileParserBug/Bad_NCDFE_Msg.java won't compile
• ... and 168 more: https://git.openjdk.org/jdk/compare/b5909cabeef22954f4d9c642b1cbf288b3454562...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk[bot]]

@rwestrel 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[bot]]

Webrevs

• 02: Full - Incremental (4029073b)
• 01: Full - Incremental (b22ba3f7)
• 00: Full (b05cf955)

[chhagedorn]

IIUC, the rational behind it is that when a CastII becomes top, then some control path must also be folded to not end up with a broken graph. And the control path to be folded should be the one on which the CastII is control dependent on with a control input?

Are there other places where a CastII is currently without control input? If so, should we fix those as well?

[rwestrel]

IIUC, the rational behind it is that when a CastII becomes top, then some control path must also be folded to not end up with a broken graph. And the control path to be folded should be the one on which the CastII is control dependent on with a control input?

Right.

Are there other places where a CastII is currently without control input? If so, should we fix those as well?

I found this one with manual inspection:

https://github.com/rwestrel/jdk/blob/b05cf955d68034ba014cc63486797876d492ca5f/src/hotspot/share/opto/loopTransform.cpp#L3007

And, yes I think it should have a control because if it's narrowing its input type, then it has to be dependent on some condition. There are also 2 in LibraryCallKit::inline_vector_insert() but I'm not sure what to do with these ones.

[chhagedorn]

Are there other places where a CastII is currently without control input? If so, should we fix those as well?

I found this one with manual inspection:

https://github.com/rwestrel/jdk/blob/b05cf955d68034ba014cc63486797876d492ca5f/src/hotspot/share/opto/loopTransform.cpp#L3007

And, yes I think it should have a control because if it's narrowing its input type, then it has to be dependent on some condition.

Okay, this looks similar the the case in the current patch. Maybe we can also fix this now?

There are also 2 in LibraryCallKit::inline_vector_insert() but I'm not sure what to do with these ones.

Hm okay, hard to tell. But are these CastII nodes even required? Can't we just set the type of the ConvL2I directly to TypeInt::BYTE and TypeInt::SHORT?

https://github.com/openjdk/jdk/blob/863b2a991df9204560c4680fc10dd0f68b260217/src/hotspot/share/opto/vectorIntrinsics.cpp#L2487-L2492

Looking at the history, it seems that the code was added when ConvL2I was still a non-type node.

Given that we could get rid of those two in inline_vector_insert() and only ending up with CastII nodes with a control input, we maybe want to force users to always set the control input. What do you think?

[rwestrel]

Given that we could get rid of those two in inline_vector_insert() and only ending up with CastII nodes with a control input, we maybe want to force users to always set the control input. What do you think?

That sounds reasonable. Does the new commit implement what you have in mind?

[chhagedorn]

Just thought about it again. What about ensuring that all ConstraintCastNode always set a control? I've had a quick look and it seems that we only have one CastPP where we do not set a control input. AFAICT, all other nodes set a control:

https://github.com/openjdk/jdk/blob/ca5a438e5a4612c66f70c70a9d425eca0e49e84d/src/hotspot/share/opto/vector.cpp#L491

[rwestrel]

Just thought about it again. What about ensuring that all ConstraintCastNode always set a control? I've had a quick look and it seems that we only have one CastPP where we do not set a control input. AFAICT, all other nodes set a control:

https://github.com/openjdk/jdk/blob/ca5a438e5a4612c66f70c70a9d425eca0e49e84d/src/hotspot/share/opto/vector.cpp#L491

Mandating control on all ConstraintCastNode sounds reasonable but I have no idea what control would make sense for the case above so maybe it's too constraining to always have to set the control.

[chhagedorn]

Mandating control on all ConstraintCastNode sounds reasonable but I have no idea what control would make sense for the case above so maybe it's too constraining to always have to set the control.

Maybe it is, I'm not sure about this case, either. But if this is the only case we could think about just adding the same assert you've inserted for CastII/LL at the other ConstraintCastNodes (except for CastPP due to this unclear case). What do you think about that? This could ensure that if someone is adding any such node without control, they need a good reason to not add a control instead of the other way round.

[rwestrel]

Mandating control on all ConstraintCastNode sounds reasonable but I have no idea what control would make sense for the case above so maybe it's too constraining to always have to set the control.

Maybe it is, I'm not sure about this case, either. But if this is the only case we could think about just adding the same assert you've inserted for CastII/LL at the other ConstraintCastNodes (except for CastPP due to this unclear case). What do you think about that? This could ensure that if someone is adding any such node without control, they need a good reason to not add a control instead of the other way round.

I added what you suggest in the commit.

[rwestrel]

@chhagedorn thanks for the review. @vnkozlov still good?

[rwestrel]

Thanks Vladimir.

[rwestrel]

/integrate

[openjdk[bot]]

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

• 34ee06f51122e8afb875cc3b75f513912272fd9b: 8332850: javac crashes if container for repeatable annotation is not found
• 00b53481064ed1ca914008d06ce5b530ca7ffe16: 8337192: [BACKOUT] JDK-8336098 G1: Refactor G1RebuildRSAndScrubTask
• 3baff2af6a30cc6cb2e0d4391db1cf7e61c33f64: 8335393: C2: assert(!had_error) failed: bad dominance
• 8081f8702a89c4895302377be62da22fc34f2c74: 8334342: Add MergeStore JMH benchmarks
• 9d8791864ec48f3321707d7f7805cd3618fc3b51: 8335191: RISC-V: verify perf of chacha20
• 6e228ce382d1fad6cba0d0df8a507e6bd32a9a4a: 8336254: Virtual thread implementation + test updates
• d3e51daf7331b84b4e78f7f10360848d7c549c1a: 8334085: Test crash: assert(thread->held_monitor_count() == 0) failed: Must be
• 0898ab7f7496689e5de52a5dc4530ca21def1fca: 8336741: Optimize LocalTime.toString with StringBuilder.repeat
• 24f67d0254c08a668d24f28ec0fa768ef10feed5: 8334232: Optimize C1 classes layout
• 021c2c36ac243009c71147072d405636cab0b12c: 8337067: Test runtime/classFileParserBug/Bad_NCDFE_Msg.java won't compile
• ... and 168 more: https://git.openjdk.org/jdk/compare/b5909cabeef22954f4d9c642b1cbf288b3454562...master

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@rwestrel Pushed as commit f61a50598958e928e9a4d81130b077cd6eaf876a.

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