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

[Philip Reames via llvm-commits]

reames added inline comments.

================
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
----------------
jfb wrote:
> 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.
Let's move this to the llvm-dev thread.

For the record, your point 3 is based on a wrong assumption.  We do support atomic loads and stores of pointers.  

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:142
@@ +141,3 @@
+        LI = convertAtomicLoadToIntegerType(LI);
+        assert(LI->getType()->isIntegerTy());
+        MadeChange = true;
----------------
jfb wrote:
> Add a string: `assert(foo && "bar");`
Will do.  For the record, I feel the message adds absolutely nothing here given the code context, but I don't care enough to argue the point.

================
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);
----------------
jfb wrote:
> 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).
I'll switch methods and add the first assert since it's slightly non-obvious.  The power of two is enforced by the verifier.  


http://reviews.llvm.org/D15471