Hi,

I'm currently re-writing core.atomics for GDC to switch from the old GCC __sync_* builtins to the new (more compatible with how core.atomics is supposed to function) __atomic_* builtins.

https://github.com/D-Programming-GDC/GDC/pull/111

One thing I've noticed in my reading of this, is that the following are accepted as valid, but makes no sense.

---
atomicStore!(MemoryOrder.acq)(var, val);

var = atomicLoad!(MemoryOrder.rel)(val);
---

I'd like to suggest that it should be considered completely valid for both cases to throw a compilation error (using static assert).  However I'd like the core druntime team to be on board with this.

Regards
Iain.

On Friday, 3 July 2015 at 17:51:17 UTC, Iain Buclaw wrote:
> Hi,
>
> I'm currently re-writing core.atomics for GDC to switch from the old GCC __sync_* builtins to the new (more compatible with how core.atomics is supposed to function) __atomic_* builtins.
>
> https://github.com/D-Programming-GDC/GDC/pull/111
>
> One thing I've noticed in my reading of this, is that the following are accepted as valid, but makes no sense.
>
> ---
> atomicStore!(MemoryOrder.acq)(var, val);
>
> var = atomicLoad!(MemoryOrder.rel)(val);
> ---
>
> I'd like to suggest that it should be considered completely valid for both cases to throw a compilation error (using static assert).  However I'd like the core druntime team to be on board with this.
>
> Regards
> Iain.

IIRC these flat out error on LDC, I meant to submit a bug report but I forgot until I read this.

On 4 Jul 2015 00:50, "rsw0x via Digitalmars-d" <digitalmars-d@puremagic.com> wrote:
>
> On Friday, 3 July 2015 at 17:51:17 UTC, Iain Buclaw wrote:
>>
>> Hi,
>>
>> I'm currently re-writing core.atomics for GDC to switch from the old GCC
__sync_* builtins to the new (more compatible with how core.atomics is supposed to function) __atomic_* builtins.
>>
>> https://github.com/D-Programming-GDC/GDC/pull/111
>>
>> One thing I've noticed in my reading of this, is that the following are
accepted as valid, but makes no sense.
>>
>> ---
>> atomicStore!(MemoryOrder.acq)(var, val);
>>
>> var = atomicLoad!(MemoryOrder.rel)(val);
>> ---
>>
>> I'd like to suggest that it should be considered completely valid for
both cases to throw a compilation error (using static assert).  However I'd like the core druntime team to be on board with this.
>>
>> Regards
>> Iain.
>
>
> IIRC these flat out error on LDC, I meant to submit a bug report but I
forgot until I read this.

Good to know, as I understand it, there's currently the following behaviour in dmd runtime.

atomicLoad!(Memoryorder.seq)  ->  issues a lock+exchange.
atomicLoad!(Memoryorder.acq)  ->  lock-free (only because of x86
guarantees).
atomicLoad!(Memoryorder.rel)  ->  issues a lock+exchange.
atomicLoad!(Memoryorder.raw)  ->  no lock.

atomicStore!(Memoryorder.seq)  ->  issues a lock+exchange.
atomicStore!(Memoryorder.acq)  ->  issues a lock+exchange.
atomicStore!(Memoryorder.rel)  ->  lock-free (only because of x86
guarantees).
atomicStore!(Memoryorder.raw)  ->  no lock.

Is it fine to consider the following (for optimisation purposes, of course
:)

atomicLoad!(Memoryorder.seq)  ->  Can be lock-free (only because of x86
guarantees), but guaranteed to do the right thing otherwise.
atomicLoad!(Memoryorder.acq)  ->  Can be lock-free (only because of x86
guarantees), but guaranteed to do the right thing otherwise.
atomicLoad!(Memoryorder.rel)  ->  static assert.
atomicLoad!(Memoryorder.raw)  ->  no lock.

atomicStore!(Memoryorder.seq)  ->  Can be lock-free (only because of x86
guarantees), but guaranteed to do the right thing otherwise.
atomicStore!(Memoryorder.acq)  ->  static assert.
atomicStore!(Memoryorder.rel)  ->  Can be lock-free (only because of x86
guarantees), but guaranteed to do the right thing otherwise.
atomicStore!(Memoryorder.raw)  ->  no lock.

Iain.

On Saturday, 4 July 2015 at 07:16:09 UTC, Iain Buclaw wrote:
> On 4 Jul 2015 00:50, "rsw0x via Digitalmars-d" <digitalmars-d@puremagic.com> wrote:
>>> [...]
> __sync_* builtins to the new (more compatible with how core.atomics is supposed to function) __atomic_* builtins.
>>> [...]
> accepted as valid, but makes no sense.
>>> [...]
> both cases to throw a compilation error (using static assert).  However I'd like the core druntime team to be on board with this.
>> [...]
> forgot until I read this.
>
> [...]

This is how it's currently implemented in C++ as of C++14, correct?
acquire semantics on a write and release semantics on a load make no sense, so this probably should be changed.

On 4 July 2015 at 10:39, rsw0x via Digitalmars-d < digitalmars-d@puremagic.com> wrote:

> On Saturday, 4 July 2015 at 07:16:09 UTC, Iain Buclaw wrote:
>
>> On 4 Jul 2015 00:50, "rsw0x via Digitalmars-d" < digitalmars-d@puremagic.com> wrote:
>>
>>> [...]
>>>>
>>> __sync_* builtins to the new (more compatible with how core.atomics is
>> supposed to function) __atomic_* builtins.
>>
>>> [...]
>>>>
>>> accepted as valid, but makes no sense.
>>
>>> [...]
>>>>
>>> both cases to throw a compilation error (using static assert).  However
>> I'd like the core druntime team to be on board with this.
>>
>>> [...]
>>>
>> forgot until I read this.
>>
>> [...]
>>
>
> This is how it's currently implemented in C++ as of C++14, correct? acquire semantics on a write and release semantics on a load make no sense, so this probably should be changed.
>


Yes, that is correct.  I think closely matching the behaviour of C++14 is the safe option given that this module shares a lot in common with std::atomic.

std::atomic::atomic_compare_exchange_strong -> core.atomic.cas std::atomic::memory_order -> core.atomic.MemoryOrder std::atomic::atomic_load -> core.atomic.atomicLoad std::atomic::atomic_store -> core.atomic.atomicStore std::atomic::atomic_thread_fence -> core.atomic.atomicFence

Iain.

On 4 Jul 2015 11:12, "Iain Buclaw" <ibuclaw@gdcproject.org> wrote:
>
> On 4 July 2015 at 10:39, rsw0x via Digitalmars-d <
digitalmars-d@puremagic.com> wrote:
>>
>> On Saturday, 4 July 2015 at 07:16:09 UTC, Iain Buclaw wrote:
>>>
>>> On 4 Jul 2015 00:50, "rsw0x via Digitalmars-d" <
digitalmars-d@puremagic.com> wrote:
>>>>>
>>>>> [...]
>>>
>>> __sync_* builtins to the new (more compatible with how core.atomics is
supposed to function) __atomic_* builtins.
>>>>>
>>>>> [...]
>>>
>>> accepted as valid, but makes no sense.
>>>>>
>>>>> [...]
>>>
>>> both cases to throw a compilation error (using static assert).  However
I'd like the core druntime team to be on board with this.
>>>>
>>>> [...]
>>>
>>> forgot until I read this.
>>>
>>> [...]
>>
>>
>> This is how it's currently implemented in C++ as of C++14, correct? acquire semantics on a write and release semantics on a load make no
sense, so this probably should be changed.
>
>
>
> Yes, that is correct.  I think closely matching the behaviour of C++14 is
the safe option given that this module shares a lot in common with std::atomic.
>
> std::atomic::atomic_compare_exchange_strong -> core.atomic.cas std::atomic::memory_order -> core.atomic.MemoryOrder std::atomic::atomic_load -> core.atomic.atomicLoad std::atomic::atomic_store -> core.atomic.atomicStore std::atomic::atomic_thread_fence -> core.atomic.atomicFence
>
> Iain.
>
>

OK, I've added static asserts to my PR.

Iain.

On Saturday, 4 July 2015 at 10:47:17 UTC, Iain Buclaw wrote:
>>>> both cases to throw a compilation error (using static assert).  However
> I'd like the core druntime team to be on board with this.

It makes sense to disallow them, but I wonder why they are allowed in C++.

On 8 July 2015 at 10:46, Martin Nowak via Digitalmars-d < digitalmars-d@puremagic.com> wrote:

> On Saturday, 4 July 2015 at 10:47:17 UTC, Iain Buclaw wrote:
>
>> both cases to throw a compilation error (using static assert).  However
>>>>>
>>>> I'd like the core druntime team to be on board with this.
>>
>
> It makes sense to disallow them, but I wonder why they are allowed in C++.
>

They *aren't* allowed in C++.

GCC will give you a compiler warning if you attempt it, and stdc++ should invoke a cxx_assert on it's use.

Iain

On Wednesday, 8 July 2015 at 13:58:24 UTC, Iain Buclaw wrote:
>
> GCC will give you a compiler warning if you attempt it, and stdc++ should invoke a cxx_assert on it's use.

I was under the impression that I died them once with no effect (might have been clang).
In any case, feel free to go forward with a static assert check.