RFR: 8335747: C2: fix overflow case for LoopLimit with constant inputs
Emanuel Peter
epeter at openjdk.org
Fri Jan 10 07:15:06 UTC 2025
`LoopLimitNode::Value` tries to constant-fold when it has constant inputs. However, there can be an overflow in the int-computation, but we check for it with `if (final_con == (jlong)final_int) {` and do not constant fold in that case.
However, there was an `assert` that checked that such an overflow would never be encountered. We already had to make an exception for this assert during PhaseCCP with [JDK-8309266](https://bugs.openjdk.org/browse/JDK-8335676).
Why did we not hit this assert before?
`LoopLimitNode` needs to have constant inputs. We used to assume that if the constants would lead to an overflow, then the loop-limit-check would also get similar constants, and detect that `limit <= max_int-stride` does not hold, and it would constant-fold away the loop, together with the `LoopLimitNode`.
But now we found a second case:
https://github.com/openjdk/jdk/blob/d93d1a3b58728f7978bbd5824b1bf6493b42561e/src/hotspot/share/opto/loopnode.cpp#L2555-L2563
In `PhaseIdealLoop::split_thru_phi`, we temporarily split the `LoopLimitNode` through the phi, generating a new `LoopLimitNode` for each branch of the `phi`. We then call `Value` on it to see if that leads us to constant fold one of the branches, which would be considered a "win".
https://github.com/openjdk/jdk/blob/d93d1a3b58728f7978bbd5824b1bf6493b42561e/src/hotspot/share/opto/loopopts.cpp#L87-L105
In the regression test, we have this example:
https://github.com/openjdk/jdk/blob/d93d1a3b58728f7978bbd5824b1bf6493b42561e/test/hotspot/jtreg/compiler/loopopts/TestLoopLimitOverflowDuringSplitThruPhi.java#L44-L69
We generate a temporary clone of `LoopLimitNode(init=0, limit=x, stride=4)` (would not constant fold because of variable `x = Phi(1000, 2147483647)`), which happens to be `LoopLimitNode(init=0, limit=2147483647, stride=4)`. We evaluate `Value` on this temporary clone, and hit the overflow case.
Why is it ok to just remove the assert and allow `LoopLimitNode` to overflow?
We still have the loop limit check, which checks that `limit <= max_int-stride`, and this means we would never enter the loop if we took the `Phi` branch that led to the overflow.
I could not just remove the assert, because in `LoopLimitNode::Ideal` we have this (strange?) check that does not optimize the `LoopLimitNode` if the inputs are constants:
if (in(Init)->is_Con() && in(Limit)->is_Con())
return nullptr; // Value
The assumption seems to be that we want `Value` to do the constant folding here - but of course we did not constant-fold because we had detected the overflow in `Value`. Not optimizing further here has a unfortunate consequence: on platforms that do not have `LoopLimit` implemented in the backend directly, we would have "lowered" the `LoopLimit` further down into `ConvI2L, SubL, AddL, DivL, ConvL2I ...` nodes. With this check, we do never lower it, and end up with a "bad AD", i.e. compilation bailout in product.
I think this check can reasonably be removed, because `Value` should be called before `Ideal` anyway, and so if we can constand fold because of constant inputs, we would have already done so.
Note, that the lowering is delayed until `post_loop_opts_phase`, but we never did `record_for_post_loop_opts_igvn`, and so it was not guaranteed that we actually ever processed the `LoopLimitNode` again, which would mean we got "bad AD" again, i.e. compilation bailout in product.
-------------
Commit messages:
- typo in comment
- copyright
- Merge branch 'master' into JDK-8335747-LoopLimitNode-overflow
- JDK-8335747
Changes: https://git.openjdk.org/jdk/pull/23024/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23024&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8335747
Stats: 85 lines in 2 files changed: 76 ins; 3 del; 6 mod
Patch: https://git.openjdk.org/jdk/pull/23024.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/23024/head:pull/23024
PR: https://git.openjdk.org/jdk/pull/23024
More information about the hotspot-compiler-dev
mailing list