[PATCH] D17413: [IR] Extend cmpxchg to allow pointer type operands

[Philip Reames via llvm-commits]

reames added inline comments.

================
Comment at: lib/AsmParser/LLParser.cpp:5912
@@ -5911,3 +5907,1 @@
-                         " integer");
-
   AtomicCmpXchgInst *CXI = new AtomicCmpXchgInst(
----------------
jfb wrote:
> I think this is too permissive, store does:
> ```
>   if (!Val->getType()->isFirstClassType())
>     return Error(Loc, "store operand must be a first class value");
> ```
> The change is also allowing pointer types. Should this be a separate change? At the minimum it has to be tested.
I was a bit confused why this code was here at all.  Shouldn't this just be caught by the Verifier?  It seems to have the checks duplicated twice.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:173
@@ +172,3 @@
+    } else if (CASI) {
+      // TODO: handle floating point here too
+      if (CASI->getCompareOperand()->getType()->isPointerTy() ) {
----------------
jfb wrote:
> FP can fall into the same helper? Though the helper will need to support FP as well.
That's the intent.  I just wanted to do that as a separate change.

================
Comment at: test/Transforms/AtomicExpand/X86/expand-atomic-non-integer.ll:124
@@ +123,2 @@
+}
+
----------------
jfb wrote:
> Add a test with volatile and addrspace, make sure they're kept.
will do


http://reviews.llvm.org/D17413