RFR: JDK-8272574: Crashes in PhaseIdealLoop::build_loop_late_post_work [v2]

[Christian Hagedorn]

On Tue, 17 Aug 2021 12:42:49 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:
> 
>   Fix version

> So the range check of outputArray[twoIdx] floats out of the inner for loop, but have a dependency of node 398 LoadI, whose control input is node 207 (loop predicate of b < inputArray.length), and node 207 is also dominated by the predicate insertion point. In the later stage, the optimization found this cycle dependency makes a bad graph.

> So, the solution is very straight forward: stop the predication to be performed if it has a control which is dominated by the predication point. But the implementation seems to have some latent problem.

I'm not sure if this is too restrictive to not apply loop predication at all. It seems to me that this `248 RangeCheck` could indeed be moved out of the loop but this pin of `398 LoadI` does not allow it which is unfortunate. Can we fix that somehow? Are we too strict when setting the control input in `LoadNode::split_through_phi()`?

I'm currently looking at:
https://github.com/openjdk/jdk/blob/30b0f820cec12b6da62229fe78a528ab3ad0d134/src/hotspot/share/opto/memnode.cpp#L1627-L1629
wondering if we could set the control input of the cloned load to the same control input as the memory input of the phi (if the control input of the old load was the region of the phi through which we split)? Something like this:

      if (this->in(0) == region) {
        if (mem->is_Phi() && (mem->in(0) == region) && mem->in(i)->in(0) != NULL) {
          x->set_req(0, mem->in(i)->in(0)); // Use same control as memory
        } else {
          x->set_req(0, in);
        }
      }

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

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