RFR: 8319793: C2 compilation fails with "Bad graph detected in build_loop_late" after JDK-8279888 [v11]

[Christian Hagedorn]

On Mon, 8 Jan 2024 15:01:12 GMT, Roland Westrelin <roland at openjdk.org> wrote:

>> Range check smearing and range check predication make an array access
>> dependent on 2 (or more in the case of RC smearing) conditions. As a
>> consequence, if a range check can be eliminated because there's an
>> identical dominating range check, the control dependent nodes that
>> could float and become dependent on the dominating range check cannot
>> be allowed to float because there's a risk that they would then bypass
>> one of the checks that make the access legal.
>> 
>> `IfNode::dominated_by()` and `PhaseIdealLoop::dominated_by()` have
>> logic to prevent this: nodes that are control dependent on a range
>> check or predicate are not allowed to float. This is however not
>> sufficient as demonstrated by the test cases.
>> 
>> In `TestArrayAccessAboveRCAfterSmearingOrPredication.testRangeCheckSmearing()`:
>> 
>> 
>>                 v += array[i];
>>                 if (flag2) {
>>                     if (flag3) {
>>                         field = 0x42;
>>                     }
>>                 }
>>                 if (flagField == 1) {
>>                     v += array[i];
>>                 }
>> 
>> 
>> The range check for the second `array[i]` load is replaced by the
>> dominating range check for the first `array[i]` but because the second
>> `array[i]` load could really be dependent on multiple range checks (in
>> case smearing happened which is not the case here), c2 doesn't allow
>> the second `array[i]` to float when the second range check is
>> removed. The second `array[i]` is then control dependent on:
>> 
>> 
>>                 if (flagField == 1) {
>> 
>> 
>> which is next found to be dominated by the same test:
>> 
>> 
>>         if (flag == 1) {
>> 
>> 
>> and is removed. However nothing in `dominated_by()` treats node
>> dependent on tests that are not range check or predicates
>> specially. So the second `array[i]` is allowed to float and become
>> dependent on:
>> 
>> 
>>         if (flag == 1) {
>> 
>> 
>> which is above the range check for that access. The test method in its
>> last invocation is passed an index for the array access that's widely
>> out of range. The array load happens before the range check and
>> crashes the VM. `testLoopPredication()` is a similar test where array
>> loads become dependent on predicates and end up above range checks.
>> 
>> `TestArrayAccessCastIIAboveRC.java` is the test case from the bug
>> where for similar reasons a range check `CastII` ends up above its
>> range check, becomes top because its input becomes some integer that
>> conflicts with its...
>
> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   whitespaces

Apart from some minor comment improvement suggestions, the new comments and renaming look good.

src/hotspot/share/opto/ifnode.cpp line 569:

> 567:   igvn->rehash_node_delayed(iff);
> 568:   iff->set_req_X(1, new_bol, igvn);
> 569:   // As part of range check smearing, this range check is widen. Loads and range check Cast nodes that are control

Suggestion:

  // As part of range check smearing, this range check is widened. Loads and range check Cast nodes that are control

src/hotspot/share/opto/loopPredicate.cpp line 1300:

> 1298:   // Eliminate the old If in the loop body
> 1299:   // If a range check is eliminated, data dependent nodes (Load and range check CastII nodes) are now dependent on 2
> 1300:   // range check predicates (one for the start of the loop, one for the end) but we can only keep track of one control

To follow the naming conventions added by the changes around JDK-8288981:
Suggestion:

  // Hoisted Check Predicates (one for the start of the loop, one for the end) but we can only keep track of one control

src/hotspot/share/opto/loopopts.cpp line 356:

> 354:       _igvn.replace_input_of(cd, 0, prevdom);
> 355:       if (pin_array_nodes) {
> 356:         // Because of range check predication, Loads and range check Cast nodes that are control dependent on this range

Loop Predication?
Suggestion:

        // Because of Loop Predication, Loads and range check Cast nodes that are control dependent on this range

src/hotspot/share/opto/loopopts.cpp line 357:

> 355:       if (pin_array_nodes) {
> 356:         // Because of range check predication, Loads and range check Cast nodes that are control dependent on this range
> 357:         // check (that is about to be removed) now depend on multiple dominating range check predicates. After the

Suggestion:

        // check (that is about to be removed) now depend on multiple dominating Hoisted Check Predicates. After the

src/hotspot/share/opto/node.hpp line 1140:

> 1138:   // Returns a clone of the current node that's pinned (if the current node is not) for nodes found in array accesses
> 1139:   // (Load and range check CastII nodes).
> 1140:   // This is used when an array access is made dependent on 2 or more range checks (range check smearing or predication).

Suggestion:

  // This is used when an array access is made dependent on 2 or more range checks (range check smearing or Loop Predication).

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16886#pullrequestreview-1810631648
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1445772859
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1445770144
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1445770896
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1445771288
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1445772291