[OpaquePtr] Make atomicrmw work with opaque pointers

[aeubanks]

Summary

FullTy is only necessary when we need to figure out what type an
instruction works with given a pointer's pointee type. However, we just
end up using the value operand's type, so FullTy isn't necessary.

[dblaikie]

Looks good!

[bogner]

Looks like you've already reverted this, but I happened to bugpoint a test case for the failure if you happen to need it. The issue is with the call to popValue if the value the atomicrmw is using hasn't been seen by the parser yet.

; RUN: opt %s -o %t.bc
; RUN: opt %t.bc -o /dev/null

define void @f() {
entry:
  br label %def

use:
  %x = atomicrmw add i32 addrspace(1)* undef, i32 %y monotonic, align 4
  br label %exit

def:
  %y = add i32 undef, undef
  br i1 undef, label %use, label %exit

exit:
  ret void
}

[aeubanks]

thanks for the repro! I'll take a look at it

[aeubanks]

rebase past D103123
add previously crashing test case

[dblaikie]

Looks good, thanks!

llvm/test/Assembler/atomicrmw.ll

1–5 ↗

(On Diff #347826)

Comment might be handy to describe what situation makes this interesting/how it failed previously, or the like?

[aeubanks]

add test comment

llvm/lib/AsmParser/LLParser.cpp

if (parseTypeAndValue(Ptr, PtrLoc, PFS) ||
       parseScopeAndOrdering(true /*Always atomic*/, SSID, Ordering) ||
       parseOptionalCommaAlign(Alignment, AteExtraComma))
     return true;
 
   if (Ordering == AtomicOrdering::Unordered)
     return tokError("atomicrmw cannot be unordered");
   if (!Ptr->getType()->isPointerTy())
     return error(PtrLoc, "atomicrmw operand must be a pointer");
-  if (cast<PointerType>(Ptr->getType())->getElementType() != Val->getType())
+  if (!cast<PointerType>(Ptr->getType())
+           ->isOpaqueOrPointeeTypeMatches(Val->getType()))
     return error(ValLoc, "atomicrmw value and pointer type do not match");
 
   if (Operation == AtomicRMWInst::Xchg) {
     if (!Val->getType()->isIntegerTy() &&
         !Val->getType()->isFloatingPointTy()) {
       return error(ValLoc,
                    "atomicrmw " + AtomicRMWInst::getOperationName(Operation) +
                        " operand must be an integer or floating point type");

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

case bitc::FUNC_CODE_INST_CMPXCHG: {
       break;
     }
     case bitc::FUNC_CODE_INST_ATOMICRMW: {
       // ATOMICRMW:[ptrty, ptr, val, op, vol, ordering, ssid, align?]
       const size_t NumRecords = Record.size();
       unsigned OpNum = 0;
 
       Value *Ptr = nullptr;
-      if (getValueTypePair(Record, OpNum, NextValueNo, Ptr, &FullTy))
+      if (getValueTypePair(Record, OpNum, NextValueNo, Ptr))
         return error("Invalid record");
 
       if (!isa<PointerType>(Ptr->getType()))
         return error("Invalid record");
 
       Value *Val = nullptr;
-      if (popValue(Record, OpNum, NextValueNo,
-                   getPointerElementFlatType(FullTy), Val))
+      if (popValue(Record, OpNum, NextValueNo, nullptr, Val))
+        return error("Invalid record");
+
+      if (!cast<PointerType>(Ptr->getType())
+               ->isOpaqueOrPointeeTypeMatches(Val->getType()))
         return error("Invalid record");
 
       if (!(NumRecords == (OpNum + 4) || NumRecords == (OpNum + 5)))
         return error("Invalid record");
 
       const AtomicRMWInst::BinOp Operation =
           getDecodedRMWOperation(Record[OpNum]);
       if (Operation < AtomicRMWInst::FIRST_BINOP ||
case bitc::FUNC_CODE_INST_ATOMICRMW: {
           return Err;
       }
 
       if (!Alignment)
         Alignment =
             Align(TheModule->getDataLayout().getTypeStoreSize(Val->getType()));
 
       I = new AtomicRMWInst(Operation, Ptr, Val, *Alignment, Ordering, SSID);
-      FullTy = getPointerElementFlatType(FullTy);
       cast<AtomicRMWInst>(I)->setVolatile(IsVol);
 
       InstructionList.push_back(I);
       break;
     }
     case bitc::FUNC_CODE_INST_FENCE: { // FENCE:[ordering, ssid]
       if (2 != Record.size())
         return error("Invalid record");

llvm/lib/IR/Instructions.cpp

void AtomicRMWInst::Init(BinOp Operation, Value *Ptr, Value *Val,
   setOrdering(Ordering);
   setSyncScopeID(SSID);
   setAlignment(Alignment);
 
   assert(getOperand(0) && getOperand(1) &&
          "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 Val type!");
+  assert(cast<PointerType>(getOperand(0)->getType())
+             ->isOpaqueOrPointeeTypeMatches(getOperand(1)->getType()) &&
+         "Ptr must be a pointer to Val type!");
   assert(Ordering != AtomicOrdering::NotAtomic &&
          "AtomicRMW instructions must be atomic!");
 }
 
 AtomicRMWInst::AtomicRMWInst(BinOp Operation, Value *Ptr, Value *Val,
                              Align Alignment, AtomicOrdering Ordering,
                              SyncScope::ID SSID, Instruction *InsertBefore)
     : Instruction(Val->getType(), AtomicRMW,

llvm/lib/IR/Verifier.cpp

 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();
   PointerType *PTy = dyn_cast<PointerType>(RMWI.getOperand(0)->getType());
   Assert(PTy, "First atomicrmw operand must be a pointer.", &RMWI);
-  Type *ElTy = PTy->getElementType();
+  Type *ElTy = RMWI.getOperand(1)->getType();
   if (Op == AtomicRMWInst::Xchg) {
     Assert(ElTy->isIntegerTy() || ElTy->isFloatingPointTy(), "atomicrmw " +
            AtomicRMWInst::getOperationName(Op) +
            " operand must have integer or floating point type!",
            &RMWI, ElTy);
   } else if (AtomicRMWInst::isFPOperation(Op)) {
     Assert(ElTy->isFloatingPointTy(), "atomicrmw " +
            AtomicRMWInst::getOperationName(Op) +
            " operand must have floating point type!",
            &RMWI, ElTy);
   } else {
     Assert(ElTy->isIntegerTy(), "atomicrmw " +
            AtomicRMWInst::getOperationName(Op) +
            " operand must have integer type!",
            &RMWI, ElTy);
   }
   checkAtomicMemAccessSize(ElTy, &RMWI);
-  Assert(ElTy == RMWI.getOperand(1)->getType(),
+  Assert(PTy->isOpaqueOrPointeeTypeMatches(ElTy),
          "Argument value type does not match pointer operand type!", &RMWI,
          ElTy);
   Assert(AtomicRMWInst::FIRST_BINOP <= Op && Op <= AtomicRMWInst::LAST_BINOP,
          "Invalid binary operation!", &RMWI);
   visitInstruction(RMWI);
 }
 
 void Verifier::visitFenceInst(FenceInst &FI) {

llvm/test/Assembler/opaque-ptr.ll

 
 ; CHECK: define void @store(ptr %a, i32 %i)
 ; CHECK:     store i32 %i, ptr %a
 ; CHECK:     ret void
 define void @store(ptr %a, i32 %i) {
     store i32 %i, ptr %a
     ret void
 }
+
+; CHECK: define void @atomicrmw(ptr %a, i32 %i)
+; CHECK:     %b = atomicrmw add ptr %a, i32 %i acquire
+; CHECK:     ret void
+define void @atomicrmw(ptr %a, i32 %i) {
+    %b = atomicrmw add ptr %a, i32 %i acquire
+    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: @atomicrmw
+define void @atomicrmw(ptr %a, i32 %i) {
+    %b = atomicrmw add ptr %a, i32 %i acquire
+    ret void
+}