[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

2–6

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_ATOMICRMW: {
       if (!isa<PointerType>(Ptr->getType()))
         return error("Invalid record");
 
       Value *Val = nullptr;
       if (BitCode == bitc::FUNC_CODE_INST_ATOMICRMW_OLD) {
         if (popValue(Record, OpNum, NextValueNo,
                      getPointerElementFlatType(FullTy), Val))
           return error("Invalid record");
+        FullTy = getPointerElementFlatType(FullTy);
       } else {
         if (getValueTypePair(Record, OpNum, NextValueNo, Val))
           return error("Invalid record");
+        FullTy = Val->getType();
       }
 
       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/test/Assembler/atomicrmw.ll

+; RUN: llvm-as %s -o - | llvm-dis | FileCheck %s
+
+; Make sure that we can parse an atomicrmw with an operand defined later in the function.
+
+; CHECK: @f
+; CHECK: atomicrmw
+define void @f() {
+  entry:
+    br label %def
+
+  use:
+    %x = atomicrmw add i32* undef, i32 %y monotonic
+    ret void
+
+  def:
+    %y = add i32 undef, undef
+    br i1 undef, label %use, label %use
+}

llvm/test/Assembler/opaque-ptr.ll

 
 ; 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
 }
+
+; 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

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