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

[Doerr, Martin]

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