[14] RFR(S): 8228772: C2 compilation fails due to unschedulable graph if DominatorSearchLimit is reached

[Nils Eliasson]

Hi Tobias,

Your patch looks good, and I seek no change to the patch, but I would 
like to take the opportunity to gain some additional insight.

Doesn't the memory graph look a bit strange? If the load is control 
dependent on the membar, and the membar creates a new memory state, 
shouldn't the load actually use that state?

The basic assumption is that for all memory ops using the same memory 
state, all loads must precede any store. This case breaks that. We 
should investigate why the IR ends up like this and if it is actually 
correct.

Regards,

Nils


On 2019-07-31 14:27, Tobias Hartmann wrote:
> Hi,
>
> please review the following patch:
> https://bugs.openjdk.java.net/browse/JDK-8228772
> http://cr.openjdk.java.net/~thartmann/8228772/webrev.00/
>
> With my fix for 8224957 [1] that is out for review, we bail out of Node::dominates when encountering
> a RegionNode with more than 3 input paths. While testing, I've encountered a rare case where this
> triggered an "unschedulable graph" assert in block local scheduling. While investigating, I found
> that this issue also reproduces without [1] when we hit the DominatorSearchLimit. I was able to
> create a regression test for both scenarios (see test1 and test2/3 in TestAntiDependentMembar).
>
> The problem is that without LoadNode::split_through_phi, a LoadNode (5198) that is control dependent
> on a membar (34) is not split through it's memory phi (5194) and therefore ends up being
> anti-dependent because both the membar and the load use the same memory (through a MergeMem):
> https://bugs.openjdk.java.net/secure/attachment/83831/8228772_graph.png
>
> If PhaseCFG::insert_anti_dependences then adds a precedence edge between the membar and the load,
> the graph becomes unschedulable (I strengthened the asserts to detect this already during GCM).
>
> We should ignore the membar if the load is already control dependent on it.
>
> Thanks,
> Tobias
>
> [1] http://cr.openjdk.java.net/~thartmann/8224957/webrev.00/