[jdk16] RFR: 8258393: Shenandoah: "graph should be schedulable" assert failure

[Roland Westrelin]

On Tue, 5 Jan 2021 11:54:07 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> This is a shenandoah bug but the fix is in shared code.
>> 
>> At barrier expansion time, we need the raw memory state for the
>> control at which the barrier is expanded. Before expansion, the memory
>> graph is traversed and memory state for each control is recorded. In
>> that process, if opportunities to improve the memory graph are found,
>> the graph is modified. In the case of the failure, a raw memory load
>> was assigned a call projection as control but for that control, no
>> memory state was recorded and the fall back is to use the call's input
>> memory state. As a consequence the load's memory input is updated to a
>> wrong memory state. This causes the assert failure.
>> 
>> The call has 2 memory projections: one for the fallthrough and one for
>> the exception path. One projection is the memory state for the
>> CatchProj for the fallthrough, the other one for the CatchProj of the
>> exception path. For the control projection out of the call, there's no
>> memory state that makes sense and assigning the control projection of
>> the call as control for the load is the root cause of the
>> problem. This happens because anti-dependence analysis is too
>> conservative: a memory Phi that merges states from the exception and
>> fallthrough path is considered a dependency and that pushed the load
>> right after the call.
>> 
>> I propose to be less conservative in anti-dependence analysis for
>> Phis. For a Phi, when computing the LCA, I think it's sufficient to
>> only consider region's inputs that we actually reach by following the
>> memory edges and that's what I propose here.
>
> src/hotspot/share/opto/loopnode.cpp line 5023:
> 
>> 5021:               Node* in = s->in(j);
>> 5022:               Node* r_in = r->in(j);
>> 5023:               if (worklist.member(in) && is_dominator(early, r_in)) {
> 
> Above we check for `!sctrl->is_top()`, do we need the same check for `r_in`?

I don't think a Region can have a top input at this point (previous IGVN should have taken care of it).

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

PR: https://git.openjdk.java.net/jdk16/pull/41