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

[Vladimir Kozlov]

On 11/14/13 12:12 PM, Lindenmaier, Goetz wrote:
> What do you prefer? Two or one changes?

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

Can you leave current changes as they are and refactor code in second 
changes? We have free cycles in JPRT now so I want to push current 
version if it is okay with you.

thanks,
Vladimir

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