RFR: 8335747: C2: fix overflow case for LoopLimit with constant inputs

[Emanuel Peter]

On Mon, 13 Jan 2025 13:25:38 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> `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-8309266).
>> 
>> 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`...
>
> You are right, but I believe the resulting expansion will constant fold regardless. So, why do we need to reject constant folding of the `LoopLimitNode` in the presence of overflow?

@merykitty You are probably right, we could probably just constant-fold in `Value`. I mean is it not a little strange anyway: why do we have the optimization twice: once in `Value` and then the lowering in `Ideal`. Feels a little like duplication.

We could investigate this in a follow-up RFE, what do you think?

-------------

PR Comment: https://git.openjdk.org/jdk/pull/23024#issuecomment-2587226316