RFR: 8299975: Limit underflow protection CMoveINode in PhaseIdealLoop::do_unroll must also protect type from underflow [v4]

[Christian Hagedorn]

On Fri, 20 Jan 2023 11:11:55 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> This is a change similar to Rolands https://github.com/openjdk/jdk/pull/4388 [JDK-8267399](https://bugs.openjdk.org/browse/JDK-8267399): After the limit is modified, its type is not specific enough. Roland added a `CastII` in `PhaseIdealLoop::do_range_check` to keep the type.
>> 
>> I have to do a similar thing for `PhaseIdealLoop::do_unroll` - but I can directly constrain the type of the `CMoveI` node:
>> We decrease the limit: `limit = limit - stride`. The subtraction makes an old limit type `int:<=3` underflow to `int`. But the `CMoveI` is already there to prevent the new limit from underflowing - it just does not manage that it's condition together with the two values it conditionally picks never underflow. And so the type information becomes too wide.
>> 
>> This is a problem if we have the following loop (simplified from regression test `test_simple`):
>> 
>>     static void test_simple(boolean flag) {
>>         int x = flag ? Integer.MIN_VALUE : 3;
>>         // x has type "int:min..3" == "<=3"
>>         // Leads to Peel and 2x Unroll
>>         for (int i = 0; i < x; i++) {
>>             iArr[i * 2] = 666 + i;
>>         }
>>     }
>> 
>> First, we have a loop with Phi `0..2`.
>> 
>> for i in 0..2:  {body} i++
>> 
>> This is then peeled:
>> 
>> i = 0; {body}
>> for i in 1..2:  {body} i++
>> 
>> and 2x Unrolled
>> 
>> i = 0; {body}
>> for i in 1..2:
>>   Phi 1..2
>>   {body}
>>   i++
>>   Phi 2..2 -> collapses to constant
>>   {body}
>>   i++ -> collapses to constant int:3
>>   exit condition: i < limit   -> int:3 < limit_t
>> 
>> 
>> If the `CMoveI` cannot determine itself that the type is `int:<=3`, then the exit condition does not fold away, and we are left with a broken loop (`LoopNode` exists, but has no `Phi` it can reach from the `LoopEndNode`). This triggers a SIGSEGV if `-XX:+TraceLoopOpts` is enabled, or any other optimization that assumes the loop-phi to be present.
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   typo correction

Fix looks good to me, too! Thanks for improving the naming, comments and adding a visual help as well. That makes it much easier to follow :-)

test/hotspot/jtreg/compiler/loopopts/TestCMoveLimitType.java line 31:

> 29:  *                   -XX:CompileCommand=compileonly,compiler.loopopts.TestCMoveLimitType::test*
> 30:  *                   -XX:CompileCommand=dontinline,compiler.loopopts.TestCMoveLimitType::dontInline
> 31:  *                   -XX:RepeatCompilation=50 -XX:+StressIGVN

You could also remove `RepeatCompilation` here and in the run below. If there is a problem at some point, we would eventually catch it by re-executing the test over and over again in our testing due to having a different seed each time.

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

Marked as reviewed by chagedorn (Reviewer).

PR: https://git.openjdk.org/jdk/pull/12092