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

[Emanuel Peter]

On Sun, 12 Jan 2025 13:16:07 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`...
>
>> I think this check can reasonably be removed, because `Value` should be called before `Ideal` anyway, and so if we can constant fold because of constant inputs, we would have already done so.
> 
> I think you are mistaken here, `Ideal` is called before `Value`.
> 
>> 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.
> 
> In this case, can we return `Type::TOP`, so that if this assumption is false we will get an error?

@merykitty 
> I think you are mistaken here, Ideal is called before Value.

Yikes, I think you are right. Still, the lowering happens only after post-loop-opts phase, so the Value optimization could have constant folded it in most cases by then. I hope that is good enough. The alternative is to test-run `Value` during `Ideal`, and check if it would constant fold... but that is a little hacky too.

> In this case, can we return Type::TOP, so that if this assumption is false we will get an error?

Hmm, I'm not sure. I'm a little worried about the if here:
`int x = flag ? 1000 : 2147483647; `
The `LoopLimitNode` gets split through the `Phi` of the `Region` of this `If`. If the `LoopLimitNode` constant folds to TOP on the right branch, then the phi collapses. But the `If` here does not need to collapse. Indeed: we can take the right branch of the if, but we just cannot enter the loop after having taken it.
Also: `LoopLimitNode::Ideal` generates nodes in the lowering. Those could in principle also reach an overflow case, and in some strange case this could later constant fold. Then we would not get TOP either... I think we should give back a valid int value or range for the `Value` case as well, and not TOP. I would rather do that then possibly mess up the graph.
What do you think?

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

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