Added atomicExchange to core.atomic

[TurkeyMan]

No description provided.

[dlang-bot]

Thanks for your pull request and interest in making D better, @TurkeyMan! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	20104	enhancement	core.atomic has no exchange function

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2729"

[TurkeyMan]

What is that CI error?

[thewilsonator]

Please add "Fix Issue 20104" to the commit title so that it gets picked up by the bot. Also pure inline asm is likely do cause problems for LDC/GDC optimisers.

[TurkeyMan]

I'm just following the rest of the code in core.atomic.
There are no intrinsics for the atomic ops...

[thewilsonator]

OK. We should probably fix that at some point though.

[TurkeyMan]

Assuming that LDC/GDC have their own druntime forks, they'll need to implement this function such that it pipes through to those compilers intrinsics properly.

[ibuclaw]

There's #2596, not merged because people are unsure what to do when atomics aren't supported (currently, fake pure mutex).

[ibuclaw]

Just to make sure you've done your homework, are there any invalid memory orders for this operation? (I think I know the answer without checking anyway).

[TurkeyMan]

There are no invalid orders.
Infact, the order does not affect codegen in any way; it should be routed through to the backend to inform code mobility.

[TurkeyMan]

people are unsure what to do when atomics aren't supported

Personally, I'd rather an error in a library called 'druntime'.
If it were a feature of phobos, I'd say spinlock.

[TurkeyMan]

wtf is an atomic float?! that's not a thing!

[ibuclaw]

Personally, I'd rather an error in a library called 'druntime'.
If it were a feature of phobos, I'd say spinlock.

That would be fine if the atomic module wasn't used by druntime in critical places. But this line of discussion is best to be done in the other pr.

... Atomic exchange SGTM.

[thewilsonator]

Of course it is, how else are you supposed to do race free parallel reductions?

[TurkeyMan]

This is grueling... how many hours have I been trying to pacify the CI now? >_<

[TurkeyMan]

I don't understand what's wrong with the float test... it works for me locally :/

[thewilsonator]

Different calling conv?

[TurkeyMan]

Doesn't explain why the apveyor test fails, since that's specifically what I'm testing...

[TurkeyMan]

Also, this code shouldn't interact with the calling convention; that's handled outside the asm block.

[thewilsonator]

maybe do

auto x = atomicExchange( &atom, val );
static if (is(T == float))
    printf("%a, %a\n",x, base);
    assert(x is base, T.stringof);

and see what it thinks?

[TurkeyMan]

@WalterBright I think there might be a codegen issue with asm in functions... this here assigns RAX to a local, which emits correct code, but then the return doesn't actually return the argument correctly.

I reduced the snippet:

extern(C) float floop(float* r, float x)
{
  asm
  {
    mov EAX, x;
    mov RCX, r;
    xchg [RCX], EAX;
    mov x, EAX;
  }
  return x;
}

And compile with DMD: dmd asmtest.d -c -m64 and emit's this:

0000000000000000 <floop>:
   0:   55                      push   %rbp
   1:   48 8b ec                mov    %rsp,%rbp
   4:   48 83 ec 10             sub    $0x10,%rsp
   8:   48 89 7d f0             mov    %rdi,-0x10(%rbp)
   c:   f3 0f 11 45 f8          movss  %xmm0,-0x8(%rbp)

; the asm block
  11:   8b 45 f8                mov    -0x8(%rbp),%eax
  14:   48 8b 4d f0             mov    -0x10(%rbp),%rcx
  18:   87 01                   xchg   %eax,(%rcx)
  1a:   89 45 f8                mov    %eax,-0x8(%rbp)

; return statement doesn't repopulate xmm0
;   movss  0x8(%rbp),%xmm0   ; <- this is missing!
  1d:   c9                      leaveq
  1e:   c3                      retq

There's a line missing that should load the float back into xmm0 before returning... I suspect DMD is assuming that the asm block did not clobber x.

This is on linux... but when I build for windows, I don't see this issue.

[TurkeyMan]

Well, the CI here suggests it fails on windows too; perhaps it's just that I can't repro this issue on my local machine...?
Is it possible there's a regression in a DMD more recent than the version I'm running locally? (2.087.0 Windows)
Has anything changed recently that might affect this?

[TurkeyMan]

-m32 does work properly:

00000000 <floop>:
   0:   55                      push   %ebp
   1:   8b ec                   mov    %esp,%ebp
   3:   8b 45 0c                mov    0xc(%ebp),%eax
   6:   8b 4d 08                mov    0x8(%ebp),%ecx
   9:   87 01                   xchg   %eax,(%ecx)
   b:   89 45 0c                mov    %eax,0xc(%ebp)
   e:   d9 45 0c                flds   0xc(%ebp)  ; <- here it is
  11:   5d                      pop    %ebp
  12:   c3                      ret

So, it's just x64... and only on master DMD, my local compiler works correctly. (2.087.0 Windows)

[TurkeyMan]

Mmm, I think is codegen bug.
Fixed it with more explicit asm...