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

[Tobias Hartmann]

On Thu, 17 Dec 2020 08:20:20 GMT, Roland Westrelin <roland 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.

Otherwise looks reasonable to me.

test/hotspot/jtreg/gc/shenandoah/compiler/TestBadRawMemoryAfterCall.java line 26:

> 24: /**
> 25:  * @test
> 26:  *

Missing `@requires vm.gc.Shenandoah`

test/hotspot/jtreg/gc/shenandoah/compiler/TestBadRawMemoryAfterCall.java line 2:

> 1: /*
> 2:  * Copyright (c) 2020, Red Hat, Inc. All rights reserved.

Copyright should be updated to 2021.

src/hotspot/share/gc/shenandoah/c2/shenandoahSupport.cpp line 2465:

> 2463: #ifdef ASSERT
> 2464:   if ((ctrl->is_Proj() && ctrl->in(0)->is_Call()) ||
> 2465:           (ctrl->is_Catch() && ctrl->in(0)->in(0)->is_Call())){

Indentation is wrong and whitespace is missing 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`?

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

Marked as reviewed by thartmann (Reviewer).

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