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

[Erik Österlund]

Hi,

As promised, I wrote some verification code triggered right before code 
gen.

Updated webrev:
http://cr.openjdk.java.net/~eosterlund/8227407/webrev.02/

I verified that this verification code catches the issues I identified 
before my fix is applied, and doesn't trigger after my fix is applied.
This type of verification won't show you where you went wrong, but it 
will show you that there is a problem (somewhere) when it arises.
Managed to get through hs-tier1-6 with this patch (and verification enabled)

Thanks,
/Erik

On 2019-07-19 09:41, Erik Österlund wrote:
> Hi Stuart,
>
> Thank you for taking this for a spin. And yes, I think the appropriate 
> place to put in such verification code would be in
>
> bs->verify_gc_barriers(this, BarrierSetC2::BeforeCodeGen);
>
> which is called right after Compile::Optimize() and before lowering to 
> machine nodes.
> I'll give it a shot to add some verification code there.
>
> Thanks,
> /Erik
>
> On 2019-07-18 17:34, Stuart Monteith wrote:
>> 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
>