MemNode::unordered/release/acquire use in relation to volatile stores and loads

[Vladimir Kozlov]

Thank you, Martin

I will file bug then and fix it.

Thanks,
Vladimir

On 8/12/14 7:35 AM, Doerr, Martin wrote:
> Hi all,
>
> exactly. This should be changed as Volker has written.
> No clue why this was missed in open jdk.
>
> Please note that the 2nd piece of code should be:
> (LibraryCallKit::load_field_from_object)
>
>    if (support_IRIW_for_not_multiple_copy_atomic_cpu && is_vol) {
>      insert_mem_bar(Op_MemBarVolatile);   // StoreLoad barrier
>    }
>    // Build the load.
>    MemNode::MemOrd mo = is_vol ? MemNode::acquire : MemNode::unordered;
>    Node* loadedField = make_load(NULL, adr, type, bt, adr_type, mo, is_vol);
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: Volker Simonis [mailto:volker.simonis at gmail.com]
> Sent: Dienstag, 5. August 2014 18:11
> To: Vladimir Kozlov; David Holmes
> Cc: HotSpot Open Source Developers; Doerr, Martin
> Subject: Re: MemNode::unordered/release/acquire use in relation to volatile stores and loads
>
> On Mon, Aug 4, 2014 at 11:13 PM, Vladimir Kozlov
> <vladimir.kozlov at oracle.com> wrote:
>> I would say that you are correct (MemNode::acquire is needed for volatile
>> load).
>> Lets hear Goetz answer.
>>
>
> Hi David, Vladimir,
>
> unfortunately, Goetz is on a sabbatical leave until mid September...
>
>> Thanks,
>> Vladimir
>>
>>
>> On 8/3/14 4:51 PM, David Holmes wrote:
>>>
>>> In ./share/vm/opto/library_call.cpp
>>>
>>> Why is it that for volatile stores we have special checks for which
>>> MemNode to use:
>>>
>>>     MemNode::MemOrd mo = is_volatile ? MemNode::release :
>>> MemNode::unordered;
>>>       if (type != T_OBJECT ) {
>>>         (void) store_to_memory(control(), adr, val, type, adr_type, mo,
>>> is_volatile);
>>>       } else {
>>>
>>> but for loads it is always unordered eg:
>>>
>>>     Node* p = make_load(control(), adr, value_type, type, adr_type,
>>> MemNode::unordered, is_volatile);
>>>
>
> That seems indeed strange. Especially because in our internal SAP JVM
> version we have:
>
> Node* p = make_load(control(), adr, value_type, type, adr_type,
> is_volatile ? MemNode::acquire : MemNode::unordered, is_volatile);
>
> I've cc-ed Martin who did this change in our sources (unfortunately
> there's still a little diff between them and the OpenJDK :(
> He will be back next week and maybe he has some more insights?
>
>>> ?? I would have expected MemNode::acquire as in opto/parse3.cpp
>>>
>>>     // Build the load.
>>>     //
>>>     MemNode::MemOrd mo = is_vol ? MemNode::acquire : MemNode::unordered;
>>>     bool needs_atomic_access = is_vol || AlwaysAtomicAccesses;
>>>     Node* ld = make_load(NULL, adr, type, bt, adr_type, mo,
>>> needs_atomic_access);
>>>
>
> And there's also another instance of this pattern in the method
> LibraryCallKit::load_field_from_object() in library_call.cpp which
> should be probably fixed as well:
>
> // Build the load.
> Node* loadedField = make_load(NULL, adr, type, bt, adr_type,
> MemNode::unordered, is_vol);
>
> Regards,
> Volker
>
>>>
>>>
>>> Thanks,
>>> David
>>>
>>