RFR: JDK-8309266: C2: assert(final_con == (jlong)final_int) failed: final value should be integer [v3]

[Roland Westrelin]

On Mon, 19 Jun 2023 08:17:31 GMT, Eric Nothum <duke at openjdk.org> wrote:

>> **Acknowledgments**: Thanks to @quadhier for the preliminary work on this issue: https://github.com/openjdk/jdk/pull/14353
>> 
>> **JDK-8309266**: TestLoopLimitOverflowDuringCCP.java causes an assertion error (overflow check) in LoopLimitNode::Value. To fix the issue I added a check in LoopLimitNode::Value that verifies that the input nodes are ConI type nodes. 
>> 
>> Previously, TestLoopLimitOverflowDuringCCP would cause the assertion error in LoopLimitNode::Value, during PhaseCCP::analyze. The problem originated from PhaseCCP initializing all types to TOP, resulting in the Phi node from `int limit = flag ? 1000 : Integer.MAX_VALUE` being temporarily considered as Integer.MAX_VALUE. This happens as the Node for the Integer.MAX_VALUE case was already analyzed by CCP while the Node for the 1000 case was still initialized to TOP. When resolving the value of the Phi node, Integer.MAX_VALUE and TOP get merged to Integer.MAX_VALUE, which is then processed by LoopLimitNode::Value as a constant, resulting in the integer overflow.
>> 
>> By checking that the input nodes are ConI nodes in LoopLimitNode::Value, we avoid Phi nodes being misinterpreted during PhaseCCP. If the Phi nodes turn out to be constant they should rather be first transformed to ConI nodes.
>
> Eric Nothum has updated the pull request incrementally with one additional commit since the last revision:
> 
>    JDK-8309266: reverted previous changes. assert was adapted and in case of overflow bottom is now returned

Looks reasonable to me.

src/hotspot/share/opto/loopnode.cpp line 2316:

> 2314:     // Assert checks for overflow only if all input nodes are ConINodes, as during CCP
> 2315:     // there might be a temporary overflow from PhiNodes see JDK-8309266
> 2316:     assert(in(Init)->is_ConI() && in(Limit)->is_ConI() && in(Stride)->is_ConI() \

Do we really need a backslash here?

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

Marked as reviewed by roland (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14490#pullrequestreview-1486109092
PR Review Comment: https://git.openjdk.org/jdk/pull/14490#discussion_r1233984481