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

[JF Bastien via llvm-commits]

jfb added inline comments.

================
Comment at: lib/AsmParser/LLParser.cpp:5912
@@ -5911,3 +5907,1 @@
-                         " integer");
-
   AtomicCmpXchgInst *CXI = new AtomicCmpXchgInst(
----------------
reames wrote:
> 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.
I'm not sure which checks are expected to go in which part of LLVM :-)

The verifier seems like more of a sanity check for internals (an assert) whereas this part seems to be the syntax check on .ll files themselves (equivalent to the errors clang gives)?

It's redundant, but there seems to be a purpose to the redundancy.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:173
@@ +172,3 @@
+    } else if (CASI) {
+      // TODO: handle floating point here too
+      if (CASI->getCompareOperand()->getType()->isPointerTy() ) {
----------------
reames wrote:
> 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.
Ah OK, and it'll fail in the verifier so it's fine. Maybe just assert early here? It'll fail miserably in convertCmpXchgToIntegerType otherwise.


http://reviews.llvm.org/D17413