RFR: JDK-8272574: C2: assert(false) failed: Bad graph detected in build_loop_late [v9]

[Vladimir Kozlov]

On Mon, 23 Aug 2021 12:40:53 GMT, 王超 <github.com+25214855+casparcwang at openjdk.org> wrote:

>> Current loop predication will promote nodes(with a dependency on a later control node) to the insertion point which dominates the later control node.
>> 
>> In the following example, loopPrediction will promote node 434 to the outer loop(predicted insert point is right after node 424), and it depends on control node 207.  But node 424 dominates node 207, which means after the promotion, the cloned nodes have a control dependency on a later control node, which leads to a bad graph.
>> 
>> ![image](https://user-images.githubusercontent.com/25214855/129720970-ff65b8f4-8bef-401d-8590-54aca6de470e.png)
>> 
>> ![image](https://user-images.githubusercontent.com/25214855/129721369-4c61222b-7305-4522-9a37-e3e6e2138aa9.png)
>
> 王超 has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove un-necessary dot

Place the test into `compiler/loopopts`.

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

> 622: // Note: this function is particularly designed for loop predication. We require load_range
> 623: //       and offset to be loop invariant computed on the fly by "invar"
> 624: bool IdealLoopTree::is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, Invariance& invar, ProjNode *predicate_proj) const {

`predicate_proj` is used only for assert. Use `DEBUG_ONLY(, ProjNode *predicate_proj)`.
Unless you want to do the check in PRODUCT.

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

> 663:     return false;
> 664:   }
> 665: #ifndef PRODUCT

Should be `#ifdef ASSERT` for debug build.

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

> 673:       // point.
> 674:       // This situation can occur when pinning nodes too conservatively - can we do better?
> 675:       assert(false, "cyclic dependency prevents range check elimination");

Print useful data before assert to help debugging issue without rerunning.
Dump `offset` node, for example.

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

> 1156:     }
> 1157: #endif
> 1158:   } else if (cl != NULL && loop->is_range_check_if(iff, this, invar, predicate_proj)) {

Use DEBUG_ONLY(, predicate_proj).

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

> 738: 
> 739:   // Return TRUE if "iff" is a range check.
> 740:   bool is_range_check_if(IfNode *iff, PhaseIdealLoop *phase, Invariance& invar, ProjNode *predicate_proj) const;

Use DEBUG_ONLY(, ProjNode *predicate_proj).

src/hotspot/share/opto/memnode.cpp line 1628:

> 1626:       // Alter data node to use pre-phi inputs
> 1627:       if (this->in(0) == region) {
> 1628:         if (mem->is_Phi() && (mem->in(0) == region) && mem->in(i)->in(0) != NULL) {

I have concern about this because you can have load's address dependency on a control node below memory's control.
I saw cases when Load's address and memory control were Region through which it splits. And all controls were set  to Region's inputs.
Also consider that we did not split memory node yet. We may end up with load's clone placed above its memory.

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

PR: https://git.openjdk.java.net/jdk/pull/5142