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

[Paul Hohensee]

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> wrote:

> On Nov 3, 2014, at 7:21 PM, Erik Österlund <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.
>
>