RFR: 8058255: Native jbyte Atomic::cmpxchg for supported x86 platforms
Kim Barrett
kim.barrett at oracle.com
Mon Nov 3 22:35:04 UTC 2014
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...]
[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.
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
src/os_cpu/solaris_x86/vm/solaris_x86_32.il
79 // Support fori jbyte Atomic::cmpxchg(jbyte exchange_value,
"fori" => "for"
------------------------------------------------------------------------------
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.
More information about the hotspot-dev
mailing list