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

[Roland Westrelin]

On Wed, 6 Mar 2024 14:50:11 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> 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.
>
>> 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.
> 
> That is indeed a general problem. The situation certainly got better by removing the code that optimized cast nodes that were pinned at If Projections (https://github.com/openjdk/jdk/commit/7766785098816cfcdae3479540cdc866c1ed18ad). By pinning the casts now, you probably want to prevent the cast nodes to be pushed through nodes such that it floats "too high" and causing unforeseenable data graph folding while control is not?
> 
>> 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 see, thanks for the explanation. Then it makes sense to keep the cast node not matter what.
> 
>> 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.
> 
> I agree, this fix should use casts. Would be interesting to follow this idea in a separate RFE.

> That is indeed a general problem. The situation certainly got better by removing the code that optimized cast nodes that were pinned at If Projections ([7766785](https://github.com/openjdk/jdk/commit/7766785098816cfcdae3479540cdc866c1ed18ad)). By pinning the casts now, you probably want to prevent the cast nodes to be pushed through nodes such that it floats "too high" and causing unforeseenable data graph folding while control is not?

Something like that. I don't see how things could go wrong in this particular case so, quite possibly, the control input is useless.

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

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