[OpaquePtr] Make cmpxchg work with opaque pointers

[dblaikie]

llvm/lib/IR/Verifier.cpp

3854–3860

Is there missing testing for this? Looks like the wording/assert was changed, but no test cases were updated for this?

[aeubanks]

llvm/lib/IR/Verifier.cpp

3854–3860

I think we could ever reasonably reach these since the bitcode reader and llparser will both already give up if this were to fail. And there's a similar assert when actually creating the instructions.
So I'm not sure how to actually test this. Maybe we should just remove these?

[dblaikie]

llvm/lib/IR/Verifier.cpp

3854–3860

Yeah, if they're unreachable I'd be inclined to remove them.

The only thing that comes to mind would be in a non-asserts build, using a unit test/API testing - build it then verify. But that's testing behavior that's out-of-contract (behavior behind a failed assertion) anyway, which isn't reasonable in my opinion.

So, yeah, if you can't get there in a +Asserts build via API, bitcode, or textual IR, I'd say it's dead code & be fine with dropping it (probably do that in a separate preliminary commit, though)

[dblaikie]

Approval for this patch/the rest of it, however the dead Asserts are handled.

[aeubanks]

llvm/lib/IR/Verifier.cpp

3854–3860

sounds good, I'll delete these in a future change

llvm/lib/AsmParser/LLParser.cpp

if (SuccessOrdering == AtomicOrdering::Unordered ||
       FailureOrdering == AtomicOrdering::Unordered)
     return tokError("cmpxchg cannot be unordered");
   if (FailureOrdering == AtomicOrdering::Release ||
       FailureOrdering == AtomicOrdering::AcquireRelease)
     return tokError(
         "cmpxchg failure ordering cannot include release semantics");
   if (!Ptr->getType()->isPointerTy())
     return error(PtrLoc, "cmpxchg operand must be a pointer");
-  if (cast<PointerType>(Ptr->getType())->getElementType() != Cmp->getType())
+  if (!cast<PointerType>(Ptr->getType())
+           ->isOpaqueOrPointeeTypeMatches(Cmp->getType()))
     return error(CmpLoc, "compare value and pointer type do not match");
-  if (cast<PointerType>(Ptr->getType())->getElementType() != New->getType())
+  if (!cast<PointerType>(Ptr->getType())
+           ->isOpaqueOrPointeeTypeMatches(New->getType()))
     return error(NewLoc, "new value and pointer type do not match");
+  if (Cmp->getType() != New->getType())
+    return error(NewLoc, "compare value and new value type do not match");
   if (!New->getType()->isFirstClassType())
     return error(NewLoc, "cmpxchg operand must be a first class value");
 
   const Align DefaultAlignment(
       PFS.getFunction().getParent()->getDataLayout().getTypeStoreSize(
           Cmp->getType()));
 
   AtomicCmpXchgInst *CXI = new AtomicCmpXchgInst(

llvm/lib/IR/Instructions.cpp

void AtomicCmpXchgInst::Init(Value *Ptr, Value *Cmp, Value *NewVal,
   setFailureOrdering(FailureOrdering);
   setSyncScopeID(SSID);
   setAlignment(Alignment);
 
   assert(getOperand(0) && getOperand(1) && getOperand(2) &&
          "All operands must be non-null!");
   assert(getOperand(0)->getType()->isPointerTy() &&
          "Ptr must have pointer type!");
-  assert(getOperand(1)->getType() ==
-                 cast<PointerType>(getOperand(0)->getType())->getElementType()
-         && "Ptr must be a pointer to Cmp type!");
-  assert(getOperand(2)->getType() ==
-                 cast<PointerType>(getOperand(0)->getType())->getElementType()
-         && "Ptr must be a pointer to NewVal type!");
+  assert(cast<PointerType>(getOperand(0)->getType())
+             ->isOpaqueOrPointeeTypeMatches(getOperand(1)->getType()) &&
+         "Ptr must be a pointer to Cmp type!");
+  assert(cast<PointerType>(getOperand(0)->getType())
+             ->isOpaqueOrPointeeTypeMatches(getOperand(2)->getType()) &&
+         "Ptr must be a pointer to NewVal type!");
+  assert(getOperand(1)->getType() == getOperand(2)->getType() &&
+         "Cmp type and NewVal type must be same!");
   assert(SuccessOrdering != AtomicOrdering::NotAtomic &&
          "AtomicCmpXchg instructions must be atomic!");
   assert(FailureOrdering != AtomicOrdering::NotAtomic &&
          "AtomicCmpXchg instructions must be atomic!");
   assert(FailureOrdering != AtomicOrdering::Release &&
          FailureOrdering != AtomicOrdering::AcquireRelease &&
          "AtomicCmpXchg failure ordering cannot include release semantics");
 }

llvm/lib/IR/Verifier.cpp

void Verifier::visitAtomicCmpXchgInst(AtomicCmpXchgInst &CXI) {
   Assert(CXI.getFailureOrdering() != AtomicOrdering::Unordered,
          "cmpxchg instructions cannot be unordered.", &CXI);
   Assert(CXI.getFailureOrdering() != AtomicOrdering::Release &&
              CXI.getFailureOrdering() != AtomicOrdering::AcquireRelease,
          "cmpxchg failure ordering cannot include release semantics", &CXI);
 
   PointerType *PTy = dyn_cast<PointerType>(CXI.getOperand(0)->getType());
   Assert(PTy, "First cmpxchg operand must be a pointer.", &CXI);
-  Type *ElTy = PTy->getElementType();
-  Assert(ElTy->isIntOrPtrTy(),
-         "cmpxchg operand must have integer or pointer type", ElTy, &CXI);
-  checkAtomicMemAccessSize(ElTy, &CXI);
-  Assert(ElTy == CXI.getOperand(1)->getType(),
+  Type *ElTy = CXI.getOperand(1)->getType();
+  Assert(PTy->isOpaqueOrPointeeTypeMatches(ElTy),
          "Expected value type does not match pointer operand type!", &CXI,
          ElTy);
   Assert(ElTy == CXI.getOperand(2)->getType(),
-         "Stored value type does not match pointer operand type!", &CXI, ElTy);
+         "Stored value type does not match expected value operand type!", &CXI,
+         ElTy);
+  Assert(ElTy->isIntOrPtrTy(),
+         "cmpxchg operand must have integer or pointer type", ElTy, &CXI);
+  checkAtomicMemAccessSize(ElTy, &CXI);
   visitInstruction(CXI);
 }
 
 void Verifier::visitAtomicRMWInst(AtomicRMWInst &RMWI) {
   Assert(RMWI.getOrdering() != AtomicOrdering::NotAtomic,
          "atomicrmw instructions must be atomic.", &RMWI);
   Assert(RMWI.getOrdering() != AtomicOrdering::Unordered,
          "atomicrmw instructions cannot be unordered.", &RMWI);
   auto Op = RMWI.getOperation();

llvm/test/Assembler/opaque-ptr-cmpxchg.ll

+; RUN: not llvm-as < %s 2>&1 | FileCheck %s
+
+; CHECK: compare value and new value type do not match
+define void @cmpxchg(ptr %p, i32 %a, i64 %b) {
+    %val_success = cmpxchg ptr %p, i32 %a, i64 %b acq_rel monotonic
+    ret void
+}

llvm/test/Assembler/opaque-ptr.ll

 
 ; CHECK: define void @gep(ptr %a)
 ; CHECK:     %b = getelementptr i8, ptr %a, i32 2
 ; CHECK:     ret void
 define void @gep(ptr %a) {
     %b = getelementptr i8, ptr %a, i32 2
     ret void
 }
+
+; CHECK: define void @cmpxchg(ptr %p, i32 %a, i32 %b)
+; CHECK:     %val_success = cmpxchg ptr %p, i32 %a, i32 %b acq_rel monotonic
+; CHECK:     ret void
+define void @cmpxchg(ptr %p, i32 %a, i32 %b) {
+    %val_success = cmpxchg ptr %p, i32 %a, i32 %b acq_rel monotonic
+    ret void
+}

llvm/test/Verifier/opaque-ptr.ll

 ; RUN: opt -passes=verify -S < %s | FileCheck %s
 
 ; CHECK: @load
 define i32 @load(ptr %a) {
     %i = load i32, ptr %a
     ret i32 %i
 }
 
 ; CHECK: @store
 define void @store(ptr %a, i32 %i) {
     store i32 %i, ptr %a
     ret void
 }
+
+; CHECK: @cmpxchg
+define void @cmpxchg(ptr %p, i32 %a, i32 %b) {
+    %val_success = cmpxchg ptr %p, i32 %a, i32 %b acq_rel monotonic
+    ret void
+}