RFR(M): 8172850: Anti-dependency on membar causes crash in register allocator due to invalid instruction scheduling

[Tobias Hartmann]

Hi Vladimir,

thanks for taking a look!

On 20.01.2017 18:06, Vladimir Kozlov wrote:
> Good. But make sure that membar in GC barrier does not allow oop store to move below it. It should not since membar's memory edge should point to it but you changed address type to RAW so I am worry. Please, check.

I looked at the graph and the scheduling of instructions and couldn't find any problems. However, I'm also worried that certain optimizations may re-wire the volatile membar or the oop store such that the order is not preserved (for example, ConnectionGraph::move_inst_mem()). I've discussed this with Roland and we came to the conclusion that it's safer to back out his original enhancement (8087341) and re-implement it in JDK 10.

I've filed 8173195 for the backout change and sent it for review separately. I would still like to push the regression test and the assert changes to prevent/catch such failures in the future:
http://cr.openjdk.java.net/~thartmann/8172850/v1/webrev.01/

Thanks,
Tobias

> Otherwise changes are good.
> 
> Thanks,
> Vladimir
> 
> On 1/20/17 5:55 AM, Tobias Hartmann wrote:
>> Hi,
>>
>> please review the following fix:
>> https://bugs.openjdk.java.net/browse/JDK-8172850
>> http://cr.openjdk.java.net/~thartmann/8172850/v1/webrev.00/
>>
>> TestMembarDependencies::test1 emits a NULL check for f2 (see line 90) that is represented as CmpN(LoadN(MEM), ConN)) and matched to testN_mem_reg0(MEM) on x86 where MEM is the memory immediately after the m1() call. The problem now is that with 8172145 [1], the MemBarVolatile emitted for the GC barrier at "f1 = obj" has an anti-dependency with the LoadN(MEM). As a result, C2 cannot schedule the testN_mem_reg0 in the block where the result is needed but has to schedule it "early", right after the call to m1. We then fail because the register allocator tries to spill the flag register to keep the result of the testN instruction live until it's needed.
>>
>> A slightly different and very simple version of the test triggers a bailout in block local scheduling for similar reasons (see comments in TestMembarDependencies::test2()).
>>
>> This only affects volatile membars that are emitted for the GC barrier because they don't have a wide memory effect in the C2 IR after the fix for 8087341 [2]. Backing out 8087341 solves the problem because the membar kills all memory and the testN_mem_reg0 is wired to its memory output and must be scheduled after.
>>
>> I fixed this by propagating the adr_type of the membar to the corresponding MachNode (similar to what Roland did in the Shenandoah repository [3]) and set the adr_type of the volatile membar to AliasIdxRaw when emitted for the GC barrier. Like this, the barrier membar is never anti-dependent on any other load and testN_mem_reg0 can be scheduled in the block where the result is needed.
>>
>> I also added missing NULL initialization and asserts to catch accidental spilling of the flags register.
>>
>> Thanks to Roland for his help!
>>
>> Tested with replay compilation, regression test and RBT (running).
>>
>> Thanks,
>> Tobias
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8172145
>> [2] https://bugs.openjdk.java.net/browse/JDK-8087341
>> [3] http://hg.openjdk.java.net/shenandoah/jdk9/hotspot/rev/cb127be5c910
>>