Re: [PATCH] libstdc++: Add mem_order_hle_acquire/release to atomic.h

[Jonathan Wakely <jwakely dot gcc at gmail dot com>]

On 9 November 2012 18:48, Andi Kleen wrote:
> On Fri, Nov 09, 2012 at 06:30:14PM +0000, Jonathan Wakely wrote:
>> On 9 November 2012 17:53, Andi Kleen wrote:
>> > On Fri, Nov 09, 2012 at 05:49:43PM +0000, Jonathan Wakely wrote:
>> >> On 9 November 2012 15:06, Andi Kleen wrote:
>> >> >
>> >> > The underlying compiler supports additional __ATOMIC_HLE_ACQUIRE/RELEASE
>> >> > memmodel flags for TSX, but this was not exposed to the C++ wrapper.
>> >> > Handle it there.
>> >>
>> >> Where do these enumerator names come from?
>> >
>> > gcc has __ATOMIC_HLE_ACQUIRE/RELEASE. This was defined for gcc some time
>> > for TSX.
>>
>> Right, but those are in the reserved name space.  If the enumerator
>> names you're adding aren't in some standard (yet) then we should avoid
>> them.
>>
>> >> They're certainly not part
>> >> of the C++ standard so your patch breaks this valid C++ program:
>> >
>> > The C++ standard didn't reserve memory_order_*? Sounds like a bug in
>> > the standard to me. But ok.
>>
>> Nope, neither does C11 as far as I can see.
>>
>> Is the mask enumerator meant to be usable by users, or is it just an
>> implementation detail?
>
> It's meant to be used by users.

OK

> Is there a problem with just allowing int?

I don't think it would violate the standard (see 17.6.5.5) but I don't
like it.  It weakens the type system and allows nonsense code like
a.store(1, 2). We don't need to allow that so I see no reason to allow
it.  The memory_order type is an enumeration not an int for a good
reason.

> It's a modifier for the other orders, but not all combinations
> are valid. The compiler checks for valid combinations.
>
> So 0x10001 would conflict.

Aha, I didn't realise they are modifiers.

So please bear with me while I try to understand what these flags are
for - I haven't followed the HLE work and don't know where they're
documented (if at all.)

Is it valid to call a.store(1, memory_order_hle_acquire) or are they
only valid as modifiers on other memory orders?

If they're not valid on their own then I would suggest the modifies
should be a separate type, and overload operator| like so:

enum __memory_order_hle_mod
{
    __memory_order_hle_acquire = 0x10000,
    __memory_order_hle_release = 0x20000
};

constexpr memory_order
operator|(memory_order __m, __memory_order_hle_mod __mod)
{
    return memory_order(__m | __mod);
}

This ensures users *cannot* call a.store(1,
__memory_order_hle_acquire) because it's the wrong type, but they
*can* call a.store(1, memory_order_acquire|__memory_order_hle_acquire)
because that calls the overloaded operator which returns the right
type.