8323972: C2 compilation fails with assert(!x->as_Loop()->is_loop_nest_inner_loop()) failed: loop was transformed #17965

rwestrel · 2024-02-22T14:36:52Z

Long counted loop are transformed into a loop nest of 2 "regular"
loops and in a subsequent loop opts round, the inner loop is
transformed into a counted loop. The limit for the inner loop is set,
when the loop nest is created, so it's expected there's no need for a
loop limit check when the counted loop is created. The assert fires
because, when the counted loop is created, it is found that it needs a
loop limit check. The reason for that is that the limit is
transformed, between nest creation and counted loop creation, in a way
that the range of values of the inner loop's limit becomes
unknown. The limit when the nest is created is:

 111  ConL  === 0  [[ 112 ]]  #long:-9223372034707292158
 106  Phi  === 105 20 94  [[ 112 ]]  #long:9223372034707292160..9223372034707292164:www !orig=72 !jvms: TestInaccurateInnerLoopLimit::test @ bci:12 (line 40)
 112  AddL  === _ 106 111  [[ 122 ]]  !orig=[110]
 122  ConvL2I  === _ 112  [[ ]]  #int

The type of 122 is 2..6 but it is then transformed to:

 106  Phi  === 105 20 154  [[ 191 130 137 ]]  #long:9223372034707292160..9223372034707292164:www !orig=[72] !jvms: TestInaccurateInnerLoopLimit::test @ bci:12 (line 40)
 191  ConvL2I  === _ 106  [[ 196 ]]  #int
 195  ConI  === 0  [[ 196 ]]  #int:max-1
 196  SubI  === _ 195 191  [[ 201 127 ]]  !orig=[123]

That is the (ConvL2I (AddL ...)) is transformed into a (SubI (ConvL2I )). ConvL2I for an input that's out of the int range of
values returns TypeInt::INT and the bounds of the limit are lost. I
propose adding a CastII after the ConvL2I so the range of values
of the limit doesn't get lost.

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-8323972: C2 compilation fails with assert(!x->as_Loop()->is_loop_nest_inner_loop()) failed: loop was transformed (Bug - P3)

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/17965/head:pull/17965
$ git checkout pull/17965

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17965

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-02-22T14:37:46Z

👋 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 · 2024-02-22T14:38:44Z

@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 · 2024-02-22T14:41:08Z

Webrevs

00: Full (ce90a1b4)

chhagedorn · 2024-02-28T08:58:23Z

src/hotspot/share/opto/loopnode.cpp

+    // a negative stride). We add a CastII here to guarantee that, when the counted loop is created in a subsequent loop
+    // opts pass, an accurate range of values for the limits is found.
+    const TypeInt* inner_iters_actual_int_range = TypeInt::make(0, iters_limit, Type::WidenMin);
+    inner_iters_actual_int = new CastIINode(outer_head, inner_iters_actual_int, inner_iters_actual_int_range, ConstraintCastNode::UnconditionalDependency);


The fix idea looks reasonable to me. I have two questions:

Do we really need to pin the CastII here? We have not pinned the ConvL2I before. And here I think we just want to ensure that the type is not lost.

Related to the first question, could we just use a normal dependency instead?

I was also wondering if we should try to improve the type of ConvL2I and of Add/Sub (and possibly also Mul) nodes in general? For ConvL2I, we could set a better type if we know that (int)lo <= (int)hi and abs(hi - lo) <= 2^32. We still have a problem to set a better type if we have a narrow range of inputs that includes min and max (e.g. min+1, min, max, max-1). In this case, ConvL2I just uses int as type. Then we could go a step further and do the same type optimization for Add/Sub nodes by directly looking through a convert/cast node at the input type. The resulting Add/Sub range could maybe be represented by something better than int:

Example:
input type to ConvL2I: [2147483647L, 2147483648L] -> type of ConvL2I is int since we cannot represent "[max_int, min_int]" with two intervals otherwise.
AddI = ConvL2I + 2 -> type could be improved to [min_int+1,min_int+2].

But that might succeed the scope of this fix. Going with CastII for now seems to be the least risk.

Thanks for reviewing this.

The fix idea looks reasonable to me. I have two questions:

* Do we really need to pin the `CastII` here? We have not pinned the `ConvL2I` before. And here I think we just want to ensure that the type is not lost.

I think it's good practice to set the control of a cast node. It probably doesn't make much of a difference here but we had so many issues with cast nodes that not setting control on cast makes me nervous now.

* Related to the first question, could we just use a normal dependency instead?

The problem with a normal dependency is that initially the cast and its non transformed input have the same types. So, there is a chance the cast is processed by igvn before its input changes and if that happens, the cast would then be removed.

I was also wondering if we should try to improve the type of ConvL2I and of Add/Sub (and possibly also Mul) nodes in general? For ConvL2I, we could set a better type if we know that (int)lo <= (int)hi and abs(hi - lo) <= 2^32. We still have a problem to set a better type if we have a narrow range of inputs that includes min and max (e.g. min+1, min, max, max-1). In this case, ConvL2I just uses int as type. Then we could go a step further and do the same type optimization for Add/Sub nodes by directly looking through a convert/cast node at the input type. The resulting Add/Sub range could maybe be represented by something better than int:

Example: input type to ConvL2I: [2147483647L, 2147483648L] -> type of ConvL2I is int since we cannot represent "[max_int, min_int]" with two intervals otherwise. AddI = ConvL2I + 2 -> type could be improved to [min_int+1,min_int+2].

But that might succeed the scope of this fix. Going with CastII for now seems to be the least risk.

I thought about that too (I didn't go as far as you did though) and my conclusion is that the change I propose should be more robust (what if the improved type computation still misses some cases that we later find are required) and less risky.

I think it's good practice to set the control of a cast node. It probably doesn't make much of a difference here but we had so many issues with cast nodes that not setting control on cast makes me nervous now.

That is indeed a general problem. The situation certainly got better by removing the code that optimized cast nodes that were pinned at If Projections (7766785). By pinning the casts now, you probably want to prevent the cast nodes to be pushed through nodes such that it floats "too high" and causing unforeseenable data graph folding while control is not?

The problem with a normal dependency is that initially the cast and its non transformed input have the same types. So, there is a chance the cast is processed by igvn before its input changes and if that happens, the cast would then be removed.

I see, thanks for the explanation. Then it makes sense to keep the cast node not matter what.

I thought about that too (I didn't go as far as you did though) and my conclusion is that the change I propose should be more robust (what if the improved type computation still misses some cases that we later find are required) and less risky.

I agree, this fix should use casts. Would be interesting to follow this idea in a separate RFE.

That is indeed a general problem. The situation certainly got better by removing the code that optimized cast nodes that were pinned at If Projections (7766785). By pinning the casts now, you probably want to prevent the cast nodes to be pushed through nodes such that it floats "too high" and causing unforeseenable data graph folding while control is not?

Something like that. I don't see how things could go wrong in this particular case so, quite possibly, the control input is useless.

openjdk · 2024-03-06T14:52:25Z

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

8323972: C2 compilation fails with assert(!x->as_Loop()->is_loop_nest_inner_loop()) failed: loop was transformed

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

c342188: 8328074: Add jcheck whitespace checking for assembly files
3c70f26: 8328112: Remove CardTable::_guard_region
48717d6: 8326333: jshell completion on arrays is incomplete
ece4124: 8328247: Remove redundant dir for tests converted from applet to main
d32ce65: 8327651: Rename DictionaryEntry members related to protection domain
0719419: 8328236: module_entry in CDS map file has stale value
0204aac: 8328115: Convert java/awt/font/TextLayout/TestJustification.html applet test to main
9bc1b06: 8328242: Add a log area to the PassFailJFrame
65a84c2: 8328006: refactor large anonymous inner class in HtmlDocletWriter
044f4ed: 8326979: (jdeps) improve the error message for FindException caused by InvalidModuleDescriptorException
... and 436 more: https://git.openjdk.org/jdk/compare/7cd25ed605469e3946a204b7b18d975c9768f2df...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.

eme64

Looks reasonable, but these ad-hoc CastII also make me nervous.

What worries me with adding such "Ad-Hoc" CastII nodes is that elsewhere a very similar computation may not have the same tight type. And then you have a tight type somewhere, and a loose type elsewhere. This is how we get the data-flow collapsing and the cfg not folding.

eme64 · 2024-03-12T17:14:05Z

test/hotspot/jtreg/compiler/longcountedloops/TestInaccurateInnerLoopLimit.java

+    }
+
+    public static void test() {
+        for (long i = 9223372034707292164L; i > 9223372034707292158L; i += -2L) { }


I'm always amazed at how such simple tests can fail. Is there any way we can improve the test coverage for Long loops?

Fuzzer test cases that call Objects.checkIndex() I suppose would possibly catch bugs.

eme64 · 2024-03-12T17:42:49Z

@rwestrel please wait for our testing to complete, I just launched it.

rwestrel · 2024-03-18T17:22:45Z

Looks reasonable, but these ad-hoc CastII also make me nervous.

I agree with that. Still feels like the most reasonable fix for this particular issue.

rwestrel · 2024-03-18T17:23:10Z

@rwestrel please wait for our testing to complete, I just launched it.

Thanks for running it. Any update on testing?

eme64 · 2024-03-19T06:48:21Z

@rwestrel Yes, the tests are passing! Ship it! 🚢

rwestrel · 2024-03-19T07:56:02Z

@chhagedorn @eme64 thanks for the reviews!

rwestrel · 2024-03-19T07:56:07Z

/integrate

openjdk · 2024-03-19T07:56:55Z

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

c0fc956: 8328275: CodeCache::print_internals should not be called in PRODUCT code
4ef591f: 8326964: Remove Eclipse Shared Workspaces
ac5b6cb: 8327757: Convert javax/swing/JSlider/6524424/bug6524424.java applet to main
652fb3a: 8328154: Convert sun/java2d/loops/CopyAreaSpeed.java applet test to main
9059727: 8327182: Move serverAlias into the loop
2dd5fba: 8319889: Vector API tests trigger VM crashes with -XX:+StressIncrementalInlining
3f2e849: 8280392: java/awt/Focus/NonFocusableWindowTest/NonfocusableOwnerTest.java failed with "RuntimeException: Test failed."
c901da4: 8327098: GTest needs larger combination limit
9e32db2: 8328309: Remove malformed masked shift instruction selection patterns
fc0472b: 8328000: Convert /java/awt/im/8154816/bug8154816.java applet test to main
... and 455 more: https://git.openjdk.org/jdk/compare/7cd25ed605469e3946a204b7b18d975c9768f2df...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-19T07:57:01Z

@rwestrel Pushed as commit e1b0af2.

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

test & fix

ce90a1b

openjdk bot added the rfr Pull request is ready for review label Feb 22, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Feb 22, 2024

chhagedorn reviewed Feb 28, 2024

View reviewed changes

chhagedorn approved these changes Mar 6, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 6, 2024

eme64 approved these changes Mar 12, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 19, 2024

openjdk bot closed this Mar 19, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8323972: C2 compilation fails with assert(!x->as_Loop()->is_loop_nest_inner_loop()) failed: loop was transformed #17965

8323972: C2 compilation fails with assert(!x->as_Loop()->is_loop_nest_inner_loop()) failed: loop was transformed #17965

rwestrel commented Feb 22, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 22, 2024

openjdk bot commented Feb 22, 2024

mlbridge bot commented Feb 22, 2024

chhagedorn Feb 28, 2024 •

edited

Loading

rwestrel Mar 6, 2024

chhagedorn Mar 6, 2024

rwestrel Mar 12, 2024

openjdk bot commented Mar 6, 2024 •

edited

Loading

eme64 left a comment

eme64 Mar 12, 2024

rwestrel Mar 18, 2024

eme64 commented Mar 12, 2024

rwestrel commented Mar 18, 2024

rwestrel commented Mar 18, 2024

eme64 commented Mar 19, 2024

rwestrel commented Mar 19, 2024

rwestrel commented Mar 19, 2024

openjdk bot commented Mar 19, 2024

openjdk bot commented Mar 19, 2024

8323972: C2 compilation fails with assert(!x->as_Loop()->is_loop_nest_inner_loop()) failed: loop was transformed #17965

8323972: C2 compilation fails with assert(!x->as_Loop()->is_loop_nest_inner_loop()) failed: loop was transformed #17965

Conversation

rwestrel commented Feb 22, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Feb 22, 2024

openjdk bot commented Feb 22, 2024

mlbridge bot commented Feb 22, 2024

Webrevs

chhagedorn Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

rwestrel Mar 6, 2024

Choose a reason for hiding this comment

chhagedorn Mar 6, 2024

Choose a reason for hiding this comment

rwestrel Mar 12, 2024

Choose a reason for hiding this comment

openjdk bot commented Mar 6, 2024 • edited Loading

eme64 left a comment

Choose a reason for hiding this comment

eme64 Mar 12, 2024

Choose a reason for hiding this comment

rwestrel Mar 18, 2024

Choose a reason for hiding this comment

eme64 commented Mar 12, 2024

rwestrel commented Mar 18, 2024

rwestrel commented Mar 18, 2024

eme64 commented Mar 19, 2024

rwestrel commented Mar 19, 2024

rwestrel commented Mar 19, 2024

openjdk bot commented Mar 19, 2024

openjdk bot commented Mar 19, 2024

rwestrel commented Feb 22, 2024 •

edited by openjdk bot

Loading

chhagedorn Feb 28, 2024 •

edited

Loading

openjdk bot commented Mar 6, 2024 •

edited

Loading