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

[Doerr, Martin]

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/

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?

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.
>>