[PATCH] D15471: [IR] Add support for floating pointer and vector atomic loads and stores

[JF Bastien via llvm-commits]

jfb added a comment.

Does this have any implications for alias analysis, especially type-based? The C++ model means memory locations are always atomic, so this isn't un-aliasing non-atomics further, but does LLVM's AAs understand that atomics can be non-integral now?

As explained in one of my comments, I think this patch should only do FP and not vectors.


================
Comment at: docs/LangRef.rst:6995
@@ -6996,1 +6994,3 @@
+be an type whose bit width is a power of two, greater than or equal to 
+eight, and less than or equal to a target-specific size limit.
 ``align`` must be explicitly specified on atomic loads, and the load has
----------------
This isn't correct because it can't be any type: it has to be an integer, FP or vector. I'm not sure I'm really enthused by the addition of vector here because it has odd ramifications:
 * What if the vector's size isn't a power of 2?
 * What about vector atomicity and alignment? Some usecases may be OK with element-wise atomicity only (with undefined ordering).
 * What if the vector contains pointers? Right now you can't have atomic load/store of pointers, it seems odd to allow vectors of pointers.

I'd drop vectors from this patch, and clarify the documentation to mention integer and floating-point.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:142
@@ +141,3 @@
+        LI = convertAtomicLoadToIntegerType(LI);
+        assert(LI->getType()->isIntegerTy());
+        MadeChange = true;
----------------
Add a string: `assert(foo && "bar");`

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:153
@@ +152,3 @@
+        SI = convertAtomicStoreToIntegerType(SI);
+        assert(SI->getValueOperand()->getType()->isIntegerTy());
+        MadeChange = true;
----------------
Ditto.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:202
@@ +201,3 @@
+  EVT VT = TLI->getValueType(DL, T);
+  unsigned BitWidth = VT.getSizeInBits();
+  return IntegerType::get(T->getContext(), BitWidth);
----------------
Shouldn't this be `getStoreSizeInBits()`? If `getStoreSizeInBits() != getSizeInBits()` then we'll have problems because the alignment may be wrong. I'd assert that they're the same, as well as checking the number is a power of 2 (because lulz fp80).


http://reviews.llvm.org/D15471