RFR(S): 8172145: C2: anti dependence missed because store hidden by membar

[Vladimir Kozlov]

On 1/3/17 2:49 AM, Doerr, Martin wrote:
> Hi Vladimir,
>
> thank you very much for this hint. It's a better way to fix the problem. I have tested it.
> New webrev is here:
> http://cr.openjdk.java.net/~mdoerr/8172145_C2_antidep/webrev.01/

Looks good.

>
> With this change, adlc generates an appropriate function:
> const TypePtr *membar_volatileNode::adr_type() const { return TypePtr::BOTTOM; }
>
> Without this change, MachNode::adr_type() gets used which doesn't fit well to membar nodes.
>
> Do you still need information about the ideal graph?

No, I originally thought there was a problem in Ideal graph. But it was not the case.
And it fixed your problem - you said you tested it. So everything is good. I will sponsor it.

It may fix other, difficult to reproduce, problems as well since this membar is used in G1 barriers and other places.

Thanks,
Vladimir

>
> Thanks and best regards,
> Martin
>
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Montag, 2. Januar 2017 22:05
> To: Andrew Haley <aph at redhat.com>; Doerr, Martin <martin.doerr at sap.com>; 'hotspot-compiler-dev at openjdk.java.net' <hotspot-compiler-dev at openjdk.java.net>
> Subject: Re: RFR(S): 8172145: C2: anti dependence missed because store hidden by membar
>
> Hmm, may be it is because MemBarVolatile is not listed in InstructForm::is_wide_memory_kill() in adlc/formssel.cpp.
> I think we should list it in is_wide_memory_kill(). I looked on all places where it is added and all use AliasIdxBot - wide memory.
>
> Vladimir
>
> On 1/2/17 12:47 PM, Vladimir Kozlov wrote:
>> I think the problem is this:
>>
>> "The membar's adr_type is NULL so can_alias returns false and reordering is not prevented."
>>
>> It should not be NULL. Type should be TypePtr::BOTTOM or specific memory related type.
>>
>> Loads should not be moved below membar if membar is wide (TypePtr::BOTTOM, affect all memory).
>>
>> It would be interesting to see these nodes in Ideal graph. What memory connections there?
>>
>>   if (support_IRIW_for_not_multiple_copy_atomic_cpu && field->is_volatile()) {
>>     insert_mem_bar(Op_MemBarVolatile);   // StoreLoad barrier
>>
>> Node* GraphKit::insert_mem_bar(int opcode, Node* precedent) {
>>   MemBarNode* mb = MemBarNode::make(C, opcode, Compile::AliasIdxBot,
>> precedent);
>>
>> and
>>
>>   _alias_types[AliasIdxBot]->Init(AliasIdxBot, TypePtr::BOTTOM);
>>
>> So I don't get why you have "adr_type is NULL"
>>
>> Thanks,
>> Vladimir
>>
>> On 12/30/16 7:34 AM, Andrew Haley wrote:
>>> Hi,
>>>
>>> On 30/12/16 13:29, Doerr, Martin wrote:
>>>>
>>>> we found a C2 Compiler bug which leads to incorrect reordering of
>>>> memory accesses on PPC64 due to missing anti-dependency. Details are
>>>> described here: https://bugs.openjdk.java.net/browse/JDK-8172145
>>>
>>> Another missing anti-dependency!
>>>
>>>> The issue can be fixed by this webrev:
>>>> http://cr.openjdk.java.net/~mdoerr/8172145_C2_antidep/webrev.00/
>>>
>>> Wouldn't it make more sense to calculate is_Mach_MemBarVolatile lazily?
>>>
>>> Andrew.
>>>