RFR: 8227407: ZGC: C2 loads and load barriers can get separated by safepoints

[Stuart Monteith]

Hello Erik,
   I'll try this against Aarch64 and let you know what I get. I'm
thinking the safepoints could be examined in the assembler to see if
they are splitting the loads and their barriers.
Is there a pass where we could enforce this as an invariant?

BR,
   Stuart

On Thu, 18 Jul 2019 at 15:46, Erik Österlund <erik.osterlund at oracle.com> wrote:
>
> Hi,
>
> The idea of "8224675: Late GC barrier insertion for ZGC" was to expand
> load barriers much later, when loads have stopped floating around. The
> reason we wanted to do that is so that when we expand load barriers, we
> won't get safepoints between loads and their load barrier.
>
> Turns out though that we still get safepoints between loads and their
> corresponding load barriers. This happens when a load with an expanded
> barrier state ends up in the same block as a safepoint.
>
> Here are reasons why this can happen that I found:
>
> 1) We don't pin the loads to their block. By that I mean make sure that
> it has the block as in(MemNode::Control), as well as have their
> _control_dependency == Pinned. Because of that, e.g. final graph
> reshaping of CastPP nodes, move their memory (load) uses up to the
> CastPP control block. That causes the load to move in to a block that
> may have safepoints in it, breaking the invariant. I now explicitly pin
> the loads to their block during late barrier injection to prevent such
> floating.
>
> 2) Despite pinning the loads after barrier injection, there is an IGVN
> transformation that explicitly moves loads above safepoints, without
> respecting if they are pinned. I changed the LoadNode::Ideal function to
> only move loads past safepoints if they are not pinned, to prevent that.
> This is the only change affecting non-ZGC code. But it seems to me that
> floating a pinned load above a safepoint is illegal in any execution
> mode and is not a ZGC-related problem. Moving for example the G1 loads
> that checks if marking is active past a safepoint will crash the VM.
>
> 3) Late barrier expansion isn't quite late enough. Barriers are expanded
> before macro expansion. Macro nodes such as locking macro nodes may
> expand into a bunch of nodes, including safepoints. I moved late barrier
> expansion to after macro expansion to solve that. It also should be
> after macro expansion for yet another reason: macro nodes my expand to
> loads that will require barriers. For example the "array copy" basic
> clone (i.e. java.lang.Object.clone() intrinsic) node does that. Now we
> have cloning disabled in ZGC right now, but should be able to enable it
> after this. That will be a later fix though for 14.
>
> 4) The original block of the load where we inject the LoadBarrier and
> splice in a successor control block (which will end up after the
> barrier), may already have safepoints in it. I solved this issue by
> moving the safepoint node to the successor control block that we
> injected as part of barrier injection. This ensures that the load and
> the safepoint don't end up in the same block.
>
> I found these issues by injecting quite intrusive verification code into
> C2. Unfortunately I think the verification code I wrote is too intrusive
> to upstream, so I would like to upstream just the fix to 13, and rework
> less intrusive verification code for 14 when there is more time to think
> about how to do that, if that is okay. I've done what I believe is the
> minimal fix to the observed problem, with the minimal risk.
>
> The test that detected the issue originally,
> "Compact_NonbranchyTree_ArrayOf", failed with root verification errors.
> Probability of failure after Stefan Karlsson's improved verification
> code seemed to be about 25% per run. With this patch, I have run 100
> iterations without failure. I have also run this through gc-test-suite
> with -Xcomp, and I am currently running through tier-1-6.
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8227407
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8227407/webrev.01/
>
> Thanks,
> /Erik