RFR(M): 8024921: PPC64 (part 113): Extend Load and Store nodes to know about memory ordering.

[Lindenmaier, Goetz]

What do you prefer? Two or one changes?
I can take out the #defines in this one, and make a separate one
for the new node.  I think this are two different issues, so there
should be two changes. 

Best regards,
  Goetz.

-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com] 
Sent: Thursday, November 14, 2013 9:08 PM
To: Lindenmaier, Goetz
Cc: hotspot-dev at openjdk.java.net; ppc-aix-port-dev at openjdk.java.net
Subject: Re: RFR(M): 8024921: PPC64 (part 113): Extend Load and Store nodes to know about memory ordering.

On 11/14/13 6:55 AM, Lindenmaier, Goetz wrote:
> Hi,
>
> I renamed the enum MemOrd, almost as Vitaly proposed.
> I also changed the order of arguments, and added default
> argument  for require_atomic_access.
> http://cr.openjdk.java.net/~goetz/webrevs/8024921-1-ldst/

This looks good. Is it final version which I can push (need to go 
throught JPRT)? Or you want to add code from your question below.

>
> I also found a way to work around on of the PPC64 defines.
> For the others in inline_unsafe_fence I actually would like to have
> some way to distinguish them in the matcher. Then I would not need
> the defines here.
> They also differ in their meaning from the other Acquire/Release
> operations, as they all refer to a certain memory operation, but these do not.
> I would propose to either introduce MemBarAcquire/ReleaseWide
> or setting a flag in the node that indicates that the membar is
> not intended for a certain memory operation.
> What do you think?

Add new one. We already did that for MemBarAcquire/ReleaseLock. It 
simplified matcher.

Thanks,
Vladimir

>
> Best regards,
>    Goetz.
>
>
>
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Mittwoch, 13. November 2013 21:02
> To: Lindenmaier, Goetz
> Subject: Re: RFR(M): 8024921: PPC64 (part 113): Extend Load and Store nodes to know about memory ordering.
>
> On 11/13/13 9:13 AM, Lindenmaier, Goetz wrote:
>> Hi Vladimir,
>>
>>> Too much changes due to explicit params which are not used on our
>>> platforms. Can you leave current make_load()/store_to_memory() for
>>> unordered case and add new make_load_ordered() and
>>> store_to_memory_ordered() for which you pass all parameters (and they
>>> call make_load()/store_to_memory()) ?
>>>
>>> Can you explain why?:
>>> "Maintaining this change has shown very error prone if the defaults for
>>> the new field are ::unordered."
>>> Did you tried what I suggested above?
>> What happened in our code is that you add a new call to the function, we
>> integrate the change and it just compiles as the default argument is used.
>> Thus we oversaw that we had to add 'release'.
>> Also, I found errors where our 'release' was cast to Boolean and passed
>> to the newly added argument 'require_atomic_access'.
>> In all these cases you will get a compile time error without default parameters.
>> Having extra functions causes similar problems.
>> If I use default parameter 'release' and 'acquire', the generated code will
>> be correct if the parameter is left out, but I have to pass the argument in
>> the most places, anyways.
>
> Okay, can you swap it with require_atomic_access argument to keep it
> with default value?
>
> Thanks,
> Vladimir
>
>>
>>> Changes to memory nodes (and LoadNode::make()/ StoreNode::make()) are
>>> fine since we need to record ordering case and there is no other way.
>>> And it is less cases than make_load()/store_to_memory().
>> Thanks!
>>
>>> Regarding David's concern about memory barriers I think changes are fine
>>> because MemBar are preserved in IR graph. Some of them are different
>>> kind but it is fine for C2 - it just needs some barriers to preserve
>>> order of loads and stores.
>>
>>> As Vitaly I also don't know what is 'Sem'. Please, translate :)
>> As Vitaly guessed, it's short for semantic.
>>
>> Best regards and thanks for the review!
>>     Goetz.
>>
>> Thanks,
>> Vladimir
>>
>> On 11/12/13 8:30 AM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> I updated this webrev to work with the latest version of the staging repository.
>>> http://cr.openjdk.java.net/~goetz/webrevs/8024921-0-ldst/
>>>
>>> Best regards,
>>>      Goetz.
>>>
>>> From: goetz.lindenmaier at sap.com
>>> Sent: Freitag, 11. Oktober 2013 15:34
>>> To: hotspot-dev developers; ppc-aix-port-dev at openjdk.java.net; Vladimir Kozlov
>>> Subject: RFR(M): 8024921: PPC64 (part 113): Extend Load and Store nodes to know about memory ordering.
>>>
>>> Hi,
>>>
>>> I prepared a webrev for 8024921<https://bugs.openjdk.java.net/browse/JDK-8024921>Extend Load and Store nodes to know about memory ordering.
>>> This is part of the PPC port.
>>> http://cr.openjdk.java.net/~goetz/webrevs/8024921-0-ldst/
>>>
>>> For a detailed description see the text in the webrev and bug description.
>>>
>>> All this basically does is add a field to load and store nodes and
>>> change all constructor calls to set this field.  So the effect on
>>> existing platforms should be very small.  Therefore I marked this
>>> 'M', although quite some lines of code are touched.
>>>
>>> Please review and test this change.
>>> I'm happy to incorporate your comments and any improvements
>>> you propose.
>>>
>>> Best regards,
>>>      Goetz.
>>>
>>>
>>>
>>>