RFR: 8307683: Loop Predication is wrongly applied to non-RangeCheckNodes without a LoadRangeNode

[Christian Hagedorn]

On Thu, 25 May 2023 16:48:35 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

> [JDK-4809552](https://bugs.openjdk.org/browse/JDK-4809552) allowed Loop Predication to be applied to `IfNodes` that have a positive value instead of a `LoadRangeNode`:
> https://github.com/openjdk/jdk/blob/48d21bd089a3f344ee5407926f8ed2af3734d2b0/src/hotspot/share/opto/loopPredicate.cpp#L854-L862
> 
> This, however, is only correct if we have an actual `RangeCheckNode` for an array. The reason for that is that if we hoist a real range check and create a Hoisted Predicate for it, we only need to check the lower and upper bound of all array accesses (i.e. the array access of the first and the last loop iteration). All array accesses in between are implicitly covered and do not need to be checked again. 
> 
> But if we face an `IfNode` without a `LoadRangeNode`, we could be comparing anything. We do not have any guarantee that if the first and last loop iteration check succeed that the other loop iteration checks will also succeed. An example of this is shown in the test case `test()`. We wrongly create a Hoisted Range Check Predicate where the lower and upper bound are always true, but for some values of the loop induction variable, the hoisted check would actually fail. We then crash because an added Assertion Predicate exactly performs this failing check (crash with halt). Without any loop splitting (i.e. no Assertion Predicates), we have a wrong execution due to never executing the branch where we increment `iFld2` because we removed it together with the check.
> 
> Thanks,
> Christian

Thanks Tobias and Roland for the initial reviews.

After thinking about the fix again and discussing it with Roland, I updated the fix:

We disallow this pattern for an `IfNode`:

if (iv <u limit) {
  trap();
}

Because this is flipped to:

if (iv >=u limit) {

} else {
  trap();
}

to have the trap to the false projection. However, this does not match the range check pattern of a `RangeCheckNode`:

if (iv <u limit) {

} else {
  trap();
}

We should therefore bailout in these cases, regardless of whether `limit` is a positive constant or a `LoadRangeNode`. If we still created a Hoisted Range Check Predicate, it could succeed at runtime (i.e. true for the value of `iv` in the first loop iteration and true for the value of `iv` in the last loop iteration) while the check to be hoisted could fail in other loop iterations. An example for this is shown in `test()`:

// Hoisted Range Check Predicate is always true:
// iv_first_iteration >=u limit && iv_last_iteration >=u limit  <=>
// -1 >=u 100 && 999 >= u 100
for (int i = -1; i < 1000; i++) {
    if (Integer.compareUnsigned(i, 100) < 0) {
        iFld2++;
        Float.isNaN(34); // Float class is unloaded with -Xcomp -> inserts trap
    } else {
        iFld++;
    }
}

However, if `0 <= i < 100`, then the hoisted check `Integer.compareUnsigned(i, 100) < 0` would be false. We then wrongly skip the branch with the `Float.isNan(34)` trap (was removed when creating the Hoisted Range Check Predicate) and miss to execute `iFld2` leading to a wrong execution (or when splitting this loop, we halt because of an initialized Assertion Predicate which will fail).

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

PR Comment: https://git.openjdk.org/jdk/pull/14156#issuecomment-1565066702