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

James Y Knight via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 16 11:37:35 PDT 2016


jyknight added inline comments.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:115
@@ +114,3 @@
+  unsigned Align = LI->getAlignment();
+  if (Align == 0)
+    return DL.getABITypeAlignment(LI->getType());
----------------
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.
> 



================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:129
@@ +128,3 @@
+unsigned getAtomicOpAlign(AtomicRMWInst *RMWI) {
+  // TODO: This instruction has no alignment attribute, but unlike the
+  // default alignment for load/store, the default here is to assume
----------------
reames wrote:
> This seems like an inconsistency we should fix.  Could you file a bug for this so that we don't forget?
> 
> Also, this function probably makes sense to promote to getAlignment on the AtomicRMWInst class itself.  
I agree it needs to be fixed. See quote from plan above. I guess I can file a bug for it if that makes it more likely someone else will do the work and I don't end up having to...

This can't really be made a getAlignment() on the class, because that would make it even more inconsistent with load and store's getAlignment functions: those return only the user-specified alignment attribute, and don't depend on DataLayout.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:941
@@ +940,3 @@
+// memory_order_* value required by the __atomic_* libcalls.
+static int libcallAtomicModel(AtomicOrdering AO) {
+  switch (AO) {
----------------
reames wrote:
> Use an enum for the memory_order_* values.  Also, don't we have that somewhere already?
Sure, I can put it in a local enum.We wouldn't have it anywhere in LLVM, because only clang deals with these values at the moment.

I mean, I //suppose// I could use stdatomic.h's enum memory_order...but it seemed better not to.


http://reviews.llvm.org/D18200





More information about the llvm-commits mailing list