RFR(M): 8024921: PPC64 (part 113): Extend Load and Store nodes to know about memory ordering.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Fri Nov 15 01:38:48 PST 2013
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 ppc-aix-port-dev
mailing list