[PATCH] D18200: Add __atomic_* lowering to AtomicExpandPass.

[James Y Knight via llvm-commits]

jyknight added a comment.

In http://reviews.llvm.org/D18200#376555, @reames wrote:

> I would strongly prefer to see the two NFC changes separated and submitted separately.  (Assuming you're reasonable sure they are NFC, that doesn't need further review.)  Having everything in the same patch makes it harder to spot semantic changes in the diff.


Okay. I'll submit those, and then upload a new version of this patch without those changes, and with the modifications requested by reviewers.

> I have to admit, I don't really understand the distinction between atomic_*, sync_*, and the buildins even after reading the documentation.  Do you have any suggestions for further reading?


Yeah, it's pretty tricky...

So, the builtins are described in GCC's documentation:
https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html#g_t_005f_005fsync-Builtins
https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html

One thing to note there is that __sync_* builtins are now almost identical to the similarly-named __atomic_* builtins, without the last ordering parameter.

The ``__atomic_*`` library functions are described here:
https://gcc.gnu.org/wiki/Atomic/GCCMM/LIbrary

The ``__sync_*`` library functions are not really described anywhere as far as I know, they are just an implementation detail between the compiler and the compiler-support library. They were originally just the default lowering of the ``__sync_*`` builtins when there was no pattern for the target.

It's all especially confusing because the state of all this evolved a bunch between earlier versions of GCC and the 4.7/4.8 timeframe when it all sort of came together.


================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:115
@@ +114,3 @@
+  unsigned Align = LI->getAlignment();
+  if (Align == 0)
+    return DL.getABITypeAlignment(LI->getType());
----------------
reames wrote:
> jyknight wrote:
> > reames wrote:
> > > Alignment can't be zero for atomic instructions.  Make this an assert.
> > I want to change that, to allow specifying atomic IR instructions with any alignment. They'll be expanded to a libcall which uses a mutex if unaligned, of course.
> > 
> > From original plan email:
> > 
> > 
> > 
> > > A4) In LLVM, add "align" attributes to cmpxchg and atomicrmw, and allow specifying "align" values for "load atomic" and "store atomic". LLVM will lower them to the generic library calls. In clang, start lowering misaligned atomics to these llvm instructions as well.
> > > 
> > 
> > 
> This sounds like a reasonable future direction, but it is not the state as of this patch.  As a result, you'd be introducing completely dead code if you landed as is.  Please do not do this.  Introduce the assert, then change in a future commit which actually introduces the new language feature.
OK, will do.


http://reviews.llvm.org/D18200