RFR: 8058255: Native jbyte Atomic::cmpxchg for supported x86 platforms

[Erik Österlund]

Okay, thanks a lot for the reviews Paul and Kim. :)
Kim can you confirm I'm good to go? Everything you mentioned is fixed and I'm ready to go.

Thanks,

/Erik

On 05 Nov 2014, at 22:10, Paul Hohensee <paul.hohensee at gmail.com<mailto:paul.hohensee at gmail.com>> wrote:

I don't need a new webrev either, so afaic you're good to go.

Thanks,

Paul


On Tue, Nov 4, 2014 at 1:15 PM, Kim Barrett <kim.barrett at oracle.com<mailto:kim.barrett at oracle.com>> wrote:
On Nov 3, 2014, at 7:21 PM, Erik Österlund <erik.osterlund at lnu.se<mailto:erik.osterlund at lnu.se>> wrote:
>
>> [legacy issue, not in changed code]
>> I think the comment for generate_atomic_cmpxchg_long() is wrong in the
>> return value; shouldn't it be returning a jlong? Probably a C-Y bug.
>
> No generate_atomic_cmpxchg_long() is used for generating code stubs for jlong CAS. I.e. it returns the address of the generated stub rather than executing a CAS - hence the return type is correct.

The comment that I’m complaining about is the one describing the operation being supported by the generator, whose return type should be jlong, just as the corresponding return type in the comment for the new cmpxchg_byte support is jbyte.  That is,

 623   // Support for jint atomic::atomic_cmpxchg_long(jlong exchange_value,

should be “// Support for jlong …"

>> src/os_cpu/bsd_x86/vm/atomic_bsd_x86.inline.hpp
>> 96                     : "q" (exchange_value), "a" (compare_value), "r" (dest), "r" (mp)
>>
>> Why is the new byte version using "q" for exchange_value, where the
>> existing int and long versions use "r"?  [There might be a good
>> reason, and this is just my rusty assembler skills showing.]
>
> With the "q" constraint you select one of the 8-bit-addressable registers rax, rcx, rdx, rbx (as opposed to any register with "r”).

Thanks for the explanation.  I didn’t remember that at all, and the documentation I skimmed yesterday wasn’t helping.

> The compare_value is assigned to eax using "a" which is also 8-bit-addressable (al). Also cmpxchgb needs it to be in al specifically.

At least I got that part.

> The former (allocating 8-bit-addressable registers) wasn't a concern for the other variants really, but here this is pretty important for the operands of cmpxchgb. :)

Indeed.

>> ------------------------------------------------------------------------------
>>
>> src/os_cpu/bsd_x86/vm/atomic_bsd_x86.inline.hpp
>> src/os_cpu/windows_x86/vm/os_windows_x86.hpp
>>
>> The windows port seems to only support specialized cmpxchgb when
>> defined(AMD64), while the BSD/Linux variants don't have that
>> restriction.  Why this inconsistency?  Or am I missing something,
>> which seems entirely possible in this tangle.
>
> If you look closely, you will see there are two definitions - one for AMD64 using a runtime-generated code stub.
> Then there is another MSVC assembly variant for #ifndef AMD64.
> This goes perfectly consistent with e.g. the jint cmpxchg for windows way of doing things.

Oops, you are correct.

> Do you want a new webrev? (just polished comments and renamed the #define as per request)

I don’t think I need one, but others might want a closer to final version.