RFR: 8323972: C2 compilation fails with assert(!x->as_Loop()->is_loop_nest_inner_loop()) failed: loop was transformed

[Roland Westrelin]

On Wed, 28 Feb 2024 08:58:23 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Long counted loop are transformed into a loop nest of 2 "regular"
>> loops and in a subsequent loop opts round, the inner loop is
>> transformed into a counted loop. The limit for the inner loop is set,
>> when the loop nest is created, so it's expected there's no need for a
>> loop limit check when the counted loop is created. The assert fires
>> because, when the counted loop is created, it is found that it needs a
>> loop limit check. The reason for that is that the limit is
>> transformed, between nest creation and counted loop creation, in a way
>> that the range of values of the inner loop's limit becomes
>> unknown. The limit when the nest is created is:
>> 
>> 
>>  111  ConL  === 0  [[ 112 ]]  #long:-9223372034707292158
>>  106  Phi  === 105 20 94  [[ 112 ]]  #long:9223372034707292160..9223372034707292164:www !orig=72 !jvms: TestInaccurateInnerLoopLimit::test @ bci:12 (line 40)
>>  112  AddL  === _ 106 111  [[ 122 ]]  !orig=[110]
>>  122  ConvL2I  === _ 112  [[ ]]  #int
>> 
>> 
>> The type of 122 is `2..6` but it is then transformed to:
>> 
>> 
>>  106  Phi  === 105 20 154  [[ 191 130 137 ]]  #long:9223372034707292160..9223372034707292164:www !orig=[72] !jvms: TestInaccurateInnerLoopLimit::test @ bci:12 (line 40)
>>  191  ConvL2I  === _ 106  [[ 196 ]]  #int
>>  195  ConI  === 0  [[ 196 ]]  #int:max-1
>>  196  SubI  === _ 195 191  [[ 201 127 ]]  !orig=[123]
>> 
>> 
>> That is the `(ConvL2I (AddL ...))` is transformed into a `(SubI
>> (ConvL2I ))`. `ConvL2I` for an input that's out of the int range of
>> values returns TypeInt::INT and the bounds of the limit are lost. I
>> propose adding a `CastII` after the `ConvL2I` so the range of values
>> of the limit doesn't get lost.
>
> src/hotspot/share/opto/loopnode.cpp line 955:
> 
>> 953:     // opts pass, an accurate range of values for the limits is found.
>> 954:     const TypeInt* inner_iters_actual_int_range = TypeInt::make(0, iters_limit, Type::WidenMin);
>> 955:     inner_iters_actual_int = new CastIINode(outer_head, inner_iters_actual_int, inner_iters_actual_int_range, ConstraintCastNode::UnconditionalDependency);
> 
> The fix idea looks reasonable to me. I have two questions:
> - Do we really need to pin the `CastII` here? We have not pinned the `ConvL2I` before. And here I think we just want to ensure that the type is not lost.
> - Related to the first question, could we just use a normal dependency instead?
> 
> I was also wondering if we should try to improve the type of `ConvL2I` and of `Add/Sub` (and possibly also `Mul`) nodes in general? For `ConvL2I`, we could set a better type if we know that `(int)lo <= (int)hi` and `abs(hi - lo) <= 2^32`. We still have a problem to set a better type if we have a narrow range of inputs that includes `min` and `max` (e.g. `min+1, min, max, max-1`). In this case, `ConvL2I` just uses `int` as type. Then we could go a step further and do the same type optimization for `Add/Sub` nodes by directly looking through a convert/cast node at the input type. The resulting `Add/Sub` range could maybe be represented by something better than `int`:
> 
> Example:
> input type to `ConvL2I`: `[2147483647L, 2147483648L]` -> type of `ConvL2I` is `int` since we cannot represent "`[max_int, min_int]`" with two intervals otherwise.
> `AddI` = `ConvL2I` + 2 -> type could be improved to `[min_int+1,min_int+2]`.
> 
> 
> But that might succeed the scope of this fix. Going with `CastII` for now seems to be the least risk.

Thanks for reviewing this.

> The fix idea looks reasonable to me. I have two questions:
> 
>     * Do we really need to pin the `CastII` here? We have not pinned the `ConvL2I` before. And here I think we just want to ensure that the type is not lost.

I think it's good practice to set the control of a cast node. It probably doesn't make much of a difference here but we had so many issues with cast nodes that not setting control on cast makes me nervous now.

>     * Related to the first question, could we just use a normal dependency instead?

The problem with a normal dependency is that initially the cast and its non transformed input have the same types. So, there is a chance the cast is processed by igvn before its input changes and if that happens, the cast would then be removed. 

> I was also wondering if we should try to improve the type of `ConvL2I` and of `Add/Sub` (and possibly also `Mul`) nodes in general? For `ConvL2I`, we could set a better type if we know that `(int)lo <= (int)hi` and `abs(hi - lo) <= 2^32`. We still have a problem to set a better type if we have a narrow range of inputs that includes `min` and `max` (e.g. `min+1, min, max, max-1`). In this case, `ConvL2I` just uses `int` as type. Then we could go a step further and do the same type optimization for `Add/Sub` nodes by directly looking through a convert/cast node at the input type. The resulting `Add/Sub` range could maybe be represented by something better than `int`:
> 
> Example: input type to `ConvL2I`: `[2147483647L, 2147483648L]` -> type of `ConvL2I` is `int` since we cannot represent "`[max_int, min_int]`" with two intervals otherwise. `AddI` = `ConvL2I` + 2 -> type could be improved to `[min_int+1,min_int+2]`.
> 
> But that might succeed the scope of this fix. Going with `CastII` for now seems to be the least risk.

I thought about that too (I didn't go as far as you did though) and my conclusion is that the change I propose should be more robust (what if the improved type computation still misses some cases that we later find are required) and less risky.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17965#discussion_r1514532046