RFR: 7143664: Clean up OrderAccess implementations and usage

[Erik Österlund]

Hi David,

On 12/02/15 01:38, David Holmes wrote:

>>> I still think we need to say something about C++ volatile and about
>>> compiler barriers in general.
>>
>> How about something like:
>>
>> C++ volatile semantics prevent compiler re-ordering between volatile
>> memory accesses. However, reordering between non-volatile and volatile
>> memory accesses is in general undefined. For compiler reordering
>> constraints taking non-volatile memory accesses into consideration, a
>> compiler barrier has to be used instead. Note also that both volatile
>> semantics and compiler barrier do not prevent hardware reordering.
>
> Sounds great! Though perhaps add "Some compiler implementations may
> choose to enforce additional constraints beyond those required by the
> language." ( a reference to MVSC)

Sounds good, fixed!

>> But since this is nowadays supported by solaris studio too, I joined
>> them into one. Not only because it's nicer to unify, but because I need
>> the compiler barrier constraints that inline asm can give us. I don't
>> know the compiler reordering constraints are guaranteed when calling
>> external assembly. So better make it explicit and safe.
>
> Totally fine with making use of Solaris Studio's modern capabilities,
> only minor concern is no way to test gcc on Solaris. But unless someone
> else screams about this I'm happy to assume that gcc will still be okay.

Since solaris studio is now actually using GCC syntax of inline asm 
which worked well (on all other GCC variants as well), I just took for 
granted that GCC would handle its own syntax on solaris too. ;)
But it never hurts to verify!

>> How do you mean they are different? To me they look quite equivalent.
>> xchg is always implicitly locked on x86, and they both use xchg to get
>> the wanted fence semantics.
>
> Windows:
>
>     89 template<>
>     90 inline void OrderAccess::specialized_release_store_fence<jint>
> (volatile jint*   p, jint   v) {
>     91   __asm {
>     92     mov edx, p;
>     93     mov eax, v;
>     94     xchg eax, dword ptr [edx];
>     95   }
>
> Linux:
>
> ! template<>
> ! inline void OrderAccess::specialized_release_store_fence<jint>
> (volatile jint*   p, jint   v) {
>       __asm__ volatile (  "xchgl (%2),%0"
>                         : "=r" (v)
>                         : "0" (v), "r" (p)
>                         : "memory");
>     }
>
> but I'm guessing the MVSC inline assembler doesn't have the same
> capabilities as gcc hence we have to write the code to load the
> registers needed by the xchg.

Yeah pretty sure the xchg instruction assumes operands are in certain 
registers. GCC is clever enough to know the constraints of the operands 
and I'm guessing MSVC inline assembly is not, so it's made explicit. 
Although who knows, maybe it's smarter nowadays. Add that on the list of 
things to find out about windows and modern compiler support! ;)

>>> Not clear about your change to the float version of release_store_fence:
>>> a) do we need to change it?
>>> b) given what we changed it to isn't that the default ?
>>
>> I didn't want to have default specializations of jfloat/jdouble
>> equalling jint/jlong. I thought maybe some architecture might want to
>> put it in different registers or something, making a default conversion
>> to int/long maybe undesirable. So the default for jfloat/jdouble is to
>> use acquire()/release() paired with an atomic memory access. So you have
>> to explicitly specialize them to be the same as jint/jlong if wanted,
>> which I believe is what is done in the code you refer to.
>
> Okay. It was the difference between the jfloat and jdouble versions in
> the original code that threw me. But the jdouble needs to delegate so we
> end up doing an Atomic::load.

jdouble and jlong is delegated to Atomic::* in orderAccess.inline.hpp as 
they should be. It would perhaps be preferrable to eventually forward 
all atomic accesses to Atomic since it's really a different concern and 
would be good to have in one place. But the methods we need are not 
there yet, so I elected not to for now.

But yeah some jfloat vs jdouble stuff in the original code is very 
strange and inconsistent. Notably, release_store_fence on linux_x86 
properly uses compiler barriers for release for all variants except the 
awkward jfloat, meaning it does not have the protection it needs. Looks 
to me like this was a human mistake due to the high redundancy of the code.

>> If you think I'm paranoid about floats, I can of course make it a
>> default behaviour to use jint/jlong even if it isn't explicit, in the
>> same way I handled unsigned variants.
>
> No I think that is okay.

Then we keep it that way. :)

> If you can send me an incremental patch I will update the webrev.

Will do, thanks for uploading!

Thanks,
/Erik