RFR(M): 8024921: PPC64 (part 113): Extend Load and Store nodes to know about memory ordering.
Vladimir Kozlov
vladimir.kozlov at oracle.com
Fri Nov 15 11:40:35 PST 2013
Thank you, Goetz
this version works. It is in JPRT queue.
Vladimir
On 11/15/13 1:38 AM, Lindenmaier, Goetz wrote:
> Hi Vladimir,
>
> sorry for that, but that's some code not included in the builds I do.
> It's guarded by TRACE_HAVE_INTRINSICS, and there is no switch
> to turn that on in the makefiles. If I force it on, I get lots of other errors:
>
> vm/c1/c1_GraphBuilder.cpp: In member function 'bool GraphBuilder::try_inline_intrinsics(ciMethod*)':
> vm/c1/c1_GraphBuilder.cpp:3363: error: '_classID' is not a member of 'vmIntrinsics'
> vm/c1/c1_GraphBuilder.cpp:3364: error: '_threadID' is not a member of 'vmIntrinsics'
> vm/c1/c1_GraphBuilder.cpp:3369: error: '_counterTime' is not a member of 'vmIntrinsics'
>
> vm/c1/c1_LIRGenerator.cpp: In member function 'virtual void LIRGenerator::do_Intrinsic(Intrinsic*)':
> vm/c1/c1_LIRGenerator.cpp:3049: error: '_threadID' is not a member of 'vmIntrinsics'
> vm/c1/c1_LIRGenerator.cpp:3050: error: '_classID' is not a member of 'vmIntrinsics'
> vm/c1/c1_LIRGenerator.cpp:3051: error: '_counterTime' is not a member of 'vmIntrinsics'
> vm/c1/c1_LIRGenerator.cpp:3052: error: 'TRACE_TIME_METHOD' was not declared in this scope
>
> I would need access to your closed sources and JPRT to fully test this, I guess.
> I checked all the calls once more, but I can not test it, so I can not assure I got all.
> I updated the webrev
> http://cr.openjdk.java.net/~goetz/webrevs/8024921-2-ldst/
>
> Best regards and thanks for pushing 115,
> Goetz.
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Freitag, 15. November 2013 04:24
> To: Lindenmaier, Goetz
> Subject: Re: RFR(M): 8024921: PPC64 (part 113): Extend Load and Store nodes to know about memory ordering.
>
> Goetz,
>
> I have build problem in library_call.cpp.
>
> Looks like you missed several make_load(), store_to_memory() calls:
>
> src/share/vm/opto/library_call.cpp: In member function 'bool
> LibraryCallKit::inline_native_classID()':
> src/share/vm/opto/library_call.cpp:3180: error: no matching function for
> call to 'LibraryCallKit::make_load(NULL, Node*&, const TypeLong*&,
> BasicType)'
> src/share/vm/opto/library_call.cpp:3187: error: no matching function for
> call to 'LibraryCallKit::store_to_memory(Node*, Node*&, Node*&,
> BasicType, const TypePtr*&)'
>
> and several other places.
>
> Vladimir
>
> On 11/14/13 1:21 PM, Lindenmaier, Goetz wrote:
>> Hi Vladimir,
>>
>> The defines are in
>> library_call.cpp:3117, in inline_unsafe_allocate().
>>
>> here goes the webrev without the changes in inline_unsafe_allocate:
>> http://cr.openjdk.java.net/~goetz/webrevs/8024921-2-ldst/
>>
>> And here a first draft for MemBarAcquire/ReleaseWide:
>> http://cr.openjdk.java.net/~goetz/webrevs/MemBarWide/
>> But I definitely still need to test this, I'll add it into our VM
>> to get the full tests before calling for a real review.
>>
>> Best regards,
>> Goetz.
>>
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Thursday, November 14, 2013 9:22 PM
>> To: Lindenmaier, Goetz
>> Subject: Re: RFR(M): 8024921: PPC64 (part 113): Extend Load and Store nodes to know about memory ordering.
>>
>> Actually what defines you are talking about? I don't see them in these
>> changes.
>>
>> Vladimir
>>
>> On 11/14/13 12:18 PM, Vladimir Kozlov wrote:
>>> 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.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
More information about the hotspot-dev
mailing list