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

Tue Jan 9 13:54:42 UTC 2024

On Tue, 9 Jan 2024 09:28:01 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 five additional commits since the last revision:
> 
>  - Update src/hotspot/share/opto/ifnode.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update src/hotspot/share/opto/node.hpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update src/hotspot/share/opto/loopopts.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update src/hotspot/share/opto/loopopts.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>
>  - Update src/hotspot/share/opto/loopPredicate.cpp
>    
>    Co-authored-by: Christian Hagedorn <christian.hagedorn at oracle.com>

@rwestrel thanks for the update, I really like the comments now! Just one more comment suggestion and a single renaming idea.

Otherwise LGTM 😊

src/hotspot/share/opto/cfgnode.hpp line 434:

> 432:   static Node* up_one_dom(Node* curr, bool linear_only = false);
> 433:   bool is_zero_trip_guard() const;
> 434:   Node* dominated_by(Node* prev_dom, PhaseIterGVN* igvn, bool pin_array_nodes);

Suggestion:

  Node* dominated_by(Node* prev_dom, PhaseIterGVN* igvn, bool pin_array_access_nodes);

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

> 1500: 
> 1501: //------------------------------dominated_by-----------------------------------
> 1502: Node* IfNode::dominated_by(Node* prev_dom, PhaseIterGVN* igvn, bool pin_array_nodes) {

Suggestion:

Node* IfNode::dominated_by(Node* prev_dom, PhaseIterGVN* igvn, bool pin_array_access_nodes) {

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

> 1535:         // Do not rewire Div and Mod nodes which could have a zero divisor to avoid skipping their zero check.
> 1536:         igvn->replace_input_of(s, 0, data_target); // Move child to data-target
> 1537:         if (pin_array_nodes && data_target != top) {

Suggestion:

        if (pin_array_access_nodes && data_target != top) {

src/hotspot/share/opto/loopnode.hpp line 1510:

> 1508:   // Mark an IfNode as being dominated by a prior test,
> 1509:   // without actually altering the CFG (and hence IDOM info).
> 1510:   void dominated_by(IfProjNode* prevdom, IfNode* iff, bool flip = false, bool pin_array_nodes = false);

Suggestion:

  void dominated_by(IfProjNode* prevdom, IfNode* iff, bool flip = false, bool pin_array_access_nodes = false);

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

> 306: // IGVN worklist for later cleanup.  Move control-dependent data Nodes on the
> 307: // live path up to the dominating control.
> 308: void PhaseIdealLoop::dominated_by(IfProjNode* prevdom, IfNode* iff, bool flip, bool pin_array_nodes) {

Suggestion:

void PhaseIdealLoop::dominated_by(IfProjNode* prevdom, IfNode* iff, bool flip, bool pin_array_access_nodes) {

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

> 353:       assert(cd->in(0) == dp, "");
> 354:       _igvn.replace_input_of(cd, 0, prevdom);
> 355:       if (pin_array_nodes) {

Suggestion:

      if (pin_array_access_nodes) {

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16886#pullrequestreview-1811165732
Marked as reviewed by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16886#pullrequestreview-1811191003
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1446097554
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1446098611
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1446098965
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1446108306
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1446108500
PR Review Comment: https://git.openjdk.org/jdk/pull/16886#discussion_r1446108919