RFR(M): 8155949: Support relaxed semantics in cmpxchg

[David Holmes]

Hi Martin,

On 23/05/2016 7:29 PM, Doerr, Martin wrote:
> Hi David,
>
> here's the new webrev:
> http://cr.openjdk.java.net/~mdoerr/8155949_relaxed_cas/webrev.04/

There seems to be some confusion. You've moved the jbyte 
Atomic::cmpxchg_general from the .cpp file to the .inline/hpp file, but 
the comments from Andrew and Kim were about moving the unsigned 
Atomic::cmpxchg version. ??

Aside: In the changeset contributor's have to be specified by "email 
address" or "name <email address>", OpenJDK user names are not accepted. 
I think Andrew should also be listed there for the Aarch64 component.

Thanks,
David

> Btw.: The jbyte version of cmpxchg can be implemented on aarch like on ppc where we emulate the byte access by a 4 byte access (lwarx/stwcx). But that should better be done in a separate change.
>
> Thanks for your time and your support.
>
> Best regards,
> Martin
>
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Samstag, 21. Mai 2016 01:10
> To: Doerr, Martin <martin.doerr at sap.com>
> Cc: Hiroshi H Horii <HORII at jp.ibm.com>; Tim Ellison <Tim_Ellison at uk.ibm.com>; ppc-aix-port-dev at openjdk.java.net; hotspot-gc-dev at openjdk.java.net; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(M): 8155949: Support relaxed semantics in cmpxchg
>
> Hi Martin,
>
> Are you in a position to make the change now suggested by both Kim and
> Andrew? Can you also include the Aarch64 code that Andrew provided:
>
> http://cr.openjdk.java.net/~aph/8154736
>
> I'd like to get this finalized so it is ready to push as soon as the
> process allows it to.
>
> Thanks,
> David
>
> On 20/05/2016 8:03 AM, Kim Barrett wrote:
>>> On May 18, 2016, at 6:12 AM, Doerr, Martin <martin.doerr at sap.com> wrote:
>>>
>>> Hi Kim,
>>>
>>> thank you very much for the detailed review.
>>>
>>> I agree with your comments and I have made all your requested changes here:
>>> http://cr.openjdk.java.net/~goetz/wr16/8155949-relaxed_cas/webrev.03/
>>>
>>> It's correct that the change changes the semantics of the conservative cmpxchg. In case of failure, we also execute the sync instruction, now.
>>> Advantage is that the new implementation is maximum conservative by default. I think this makes sense as long as the semantics of the hotspot C++ cmpxchg are not clearly specified.
>>>
>>> For performance optimization, we should better use (or introduce additional) enum values.
>>
>> ------------------------------------------------------------------------------
>> There doesn't seem to have been any change for this earlier comment.
>>
>> src/share/vm/runtime/atomic.cpp
>> 59 unsigned Atomic::cmpxchg(unsigned int exchange_value,
>>  60                            volatile unsigned int* dest, unsigned int compare_value,
>>  61                            cmpxchg_memory_order order) {
>>
>> I'm surprised this was ever out-of-line. But with this change it's
>> quite bad to be out-of-line, as that's going to kill the constant
>> propogation of the order value.
>>
>> ------------------------------------------------------------------------------
>>
>> Other than that, looks good.
>>
>>
>>
>>