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

[Erik Österlund]

Hi Kim,

Thanks a lot for taking a look at this! :)

On 03 Nov 2014, at 23:35, Kim Barrett <kim.barrett at oracle.com> wrote:

> This is a review of the so-called "simple fix", e.g. this:
> http://cr.openjdk.java.net/~jwilhelm/8058255/webrev.03.macro
> 
> I haven't reviewed the inline assembly code very carefully.  The gcc
> versions (linux, bsd) look correct to me (subject to comments below),
> but my knowledge of both x86 assembly and gcc inline assembly are
> somewhat rusty and should be considered suspect for this purpose.  I
> only did a superficial review of the inline assembly code for Windows,
> as I'm not familiar with the syntax for that at all.
> 
> ------------------------------------------------------------------------------
> 
> src/cpu/x86/vm/stubGenerator_x86_64.cpp 
> 597   // Support for jbyte atomic::atomic_cmpxchg( [...]
> 
> Shouldn't that be atomic_cmpxchg_byte, e.g. needs "_byte" suffix?
> That would be consistent with the comment for the later
> generate_atomic_cmpxchg_long(). But maybe that comment isn't right?
> [There isn't an Atomic::atomic_cmpxchg[_long](), only
> Atomic::cmpxchg() overloads.  And since the argument types are
> explicit in these comments...]
> 

I agree - would say the other comment for jlong CAS is a bit off - there is obviously no Atomic::cmpxchg_long. Maybe it was once called that in ancient times and the comment outlived its expiry date?
Fixed it anyway...

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

> 
> ------------------------------------------------------------------------------
> 
> 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.]
> src/os_cpu/linux_x86/vm/atomic_linux_x86.inline.hpp
> Similarly here, line 96.
> 
> src/os_cpu/solaris_x86/vm/atomic_solaris_x86.inline.hpp
> Similarly here, line 231.
> 

With the "q" constraint you select one of the 8-bit-addressable registers rax, rcx, rdx, rbx (as opposed to any register with "r").
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.
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. :)

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

> 
> ------------------------------------------------------------------------------
> 
> src/os_cpu/solaris_x86/vm/solaris_x86_32.il
>  79   // Support fori jbyte Atomic::cmpxchg(jbyte exchange_value, 
> "fori" => "for"

Fixed.

> ------------------------------------------------------------------------------
> 
> VM_HAS_SPECIALIZED_BYTE_CMPXCHG
> I think I would have called this VM_HAS_SPECIALIZED_CMPXCHG_BYTE, for
> consistency with the names of the helper primitives, but that might
> just be me.  That way a case-insensitive search for "cmpxchg_byte"
> finds both this macro and those helpers.


Okay sure, fixed.

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

Thanks again Kim for looking through these changes! :)

/Erik