[OpaquePtr] Make loads and stores work with opaque pointers

[aeubanks]

Summary

Don't check that types match when the pointer operand is an opaque
pointer.

I would separate the Assembler and Verifier changes, but
verify-uselistorder in the Assembler test ends up running the verifier.

[dblaikie]

llvm/lib/AsmParser/LLParser.cpp

7533–7534

Reckon there's anything we could do to generalize these sort of tests - if we're going to add a whole bunch of them through this migration (& then remove them once we've moved over to opaque pointer types), maybe there are some good primitives we could create?

Like "hasElementTypeOrOpaque(PtrType, PointeeType)"?

[aeubanks]

factor out check for pointee type
add some more missed places (not sure why I thought the test was passing before?)

[dblaikie]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3849 ↗

(On Diff #345334)

Is this for sure equivalent to the old code? The old code looks like it's equivalent to PointerType::isValidElementType which looks different to me to isFirstClassType?

llvm/lib/IR/Verifier.cpp

3749

What's this condition for? If this is true, isOpaqueOrPointeeTypeMatches would be true and the assert would not fail, so that seems OK?

[aeubanks]

remove unnecessary check

[aeubanks]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3849 ↗

(On Diff #345334)

it matches the check in LLParser:

if (!Val->getType()->isPointerTy() || !Ty->isFirstClassType())
  return error(Loc, "load operand must be a pointer to a first class type");

The pointer should already have a valid pointee type, so that check isn't useful. isFirstClassType() seems more proper than just checking for a function type

[dblaikie]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3849 ↗

(On Diff #345334)

I'd be inclined to have that as a separate commit, perhaps? (& is it worth an assertion that those other cases covered by isLoadableOrStorableType don't come up here? (demonstrating that these two phrasings are equivalent)

[aeubanks]

isFunctionType() instead of isFirstClassType()

[aeubanks]

address comments more

[aeubanks]

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

3849 ↗

(On Diff #345334)

made this closer to how it was originally

[dblaikie]

Sounds good!

llvm/lib/AsmParser/LLParser.cpp

int LLParser::parseLoad(Instruction *&Inst, PerFunctionState &PFS) {
   if (!Val->getType()->isPointerTy() || !Ty->isFirstClassType())
     return error(Loc, "load operand must be a pointer to a first class type");
   if (isAtomic && !Alignment)
     return error(Loc, "atomic load must have explicit non-zero alignment");
   if (Ordering == AtomicOrdering::Release ||
       Ordering == AtomicOrdering::AcquireRelease)
     return error(Loc, "atomic load cannot use Release ordering");
 
-  if (Ty != cast<PointerType>(Val->getType())->getElementType()) {
+  if (!cast<PointerType>(Val->getType())->isOpaque() &&
+      Ty != cast<PointerType>(Val->getType())->getElementType()) {
     return error(
         ExplicitTypeLoc,
         typeComparisonErrorMessage(
             "explicit pointee type doesn't match operand's pointee type", Ty,
             cast<PointerType>(Val->getType())->getElementType()));
   }
   SmallPtrSet<Type *, 4> Visited;
   if (!Alignment && !Ty->isSized(&Visited))
if (parseTypeAndValue(Val, Loc, PFS) ||
       parseScopeAndOrdering(isAtomic, SSID, Ordering) ||
       parseOptionalCommaAlign(Alignment, AteExtraComma))
     return true;
 
   if (!Ptr->getType()->isPointerTy())
     return error(PtrLoc, "store operand must be a pointer");
   if (!Val->getType()->isFirstClassType())
     return error(Loc, "store operand must be a first class value");
-  if (cast<PointerType>(Ptr->getType())->getElementType() != Val->getType())
+  if (!cast<PointerType>(Ptr->getType())->isOpaque() &&
+      cast<PointerType>(Ptr->getType())->getElementType() != Val->getType())
     return error(Loc, "stored value and pointer type do not match");
   if (isAtomic && !Alignment)
     return error(Loc, "atomic store must have explicit non-zero alignment");
   if (Ordering == AtomicOrdering::Acquire ||
       Ordering == AtomicOrdering::AcquireRelease)
     return error(Loc, "atomic store cannot use Acquire ordering");
   SmallPtrSet<Type *, 4> Visited;
   if (!Alignment && !Val->getType()->isSized(&Visited))

llvm/lib/IR/Instructions.cpp

LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
                    Align Align, BasicBlock *InsertAE)
     : LoadInst(Ty, Ptr, Name, isVolatile, Align, AtomicOrdering::NotAtomic,
                SyncScope::System, InsertAE) {}
 
 LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
                    Align Align, AtomicOrdering Order, SyncScope::ID SSID,
                    Instruction *InsertBef)
     : UnaryInstruction(Ty, Load, Ptr, InsertBef) {
-  assert(Ty == cast<PointerType>(Ptr->getType())->getElementType());
+  assert(cast<PointerType>(Ptr->getType())->isOpaque() ||
+         Ty == cast<PointerType>(Ptr->getType())->getElementType());
   setVolatile(isVolatile);
   setAlignment(Align);
   setAtomic(Order, SSID);
   AssertOK();
   setName(Name);
 }
 
 LoadInst::LoadInst(Type *Ty, Value *Ptr, const Twine &Name, bool isVolatile,
                    Align Align, AtomicOrdering Order, SyncScope::ID SSID,
                    BasicBlock *InsertAE)
     : UnaryInstruction(Ty, Load, Ptr, InsertAE) {
-  assert(Ty == cast<PointerType>(Ptr->getType())->getElementType());
+  assert(cast<PointerType>(Ptr->getType())->isOpaque() ||
+         Ty == cast<PointerType>(Ptr->getType())->getElementType());
   setVolatile(isVolatile);
   setAlignment(Align);
   setAtomic(Order, SSID);
   AssertOK();
   setName(Name);
 }
 
 //===----------------------------------------------------------------------===//
 //                           StoreInst Implementation
 //===----------------------------------------------------------------------===//
 
 void StoreInst::AssertOK() {
   assert(getOperand(0) && getOperand(1) && "Both operands must be non-null!");
   assert(getOperand(1)->getType()->isPointerTy() &&
          "Ptr must have pointer type!");
-  assert(getOperand(0)->getType() ==
-                 cast<PointerType>(getOperand(1)->getType())->getElementType()
-         && "Ptr must be a pointer to Val type!");
+  assert(
+      cast<PointerType>(getOperand(1)->getType())->isOpaque() ||
+      getOperand(0)->getType() ==
+              cast<PointerType>(getOperand(1)->getType())->getElementType() &&
+          "Ptr must be a pointer to Val type!");
   assert(!(isAtomic() && getAlignment() == 0) &&
          "Alignment required for atomic store");
 }
 
 StoreInst::StoreInst(Value *val, Value *addr, Instruction *InsertBefore)
     : StoreInst(val, addr, /*isVolatile=*/false, InsertBefore) {}
 
 StoreInst::StoreInst(Value *val, Value *addr, BasicBlock *InsertAtEnd)

llvm/lib/IR/Verifier.cpp

void Verifier::visitLoadInst(LoadInst &LI) {
   }
 
   visitInstruction(LI);
 }
 
 void Verifier::visitStoreInst(StoreInst &SI) {
   PointerType *PTy = dyn_cast<PointerType>(SI.getOperand(1)->getType());
   Assert(PTy, "Store operand must be a pointer.", &SI);
-  Type *ElTy = PTy->getElementType();
-  Assert(ElTy == SI.getOperand(0)->getType(),
+  Type *ElTy = SI.getOperand(0)->getType();
+  if (!PTy->isOpaque()) {
+    Assert(ElTy == PTy->getElementType(),
            "Stored value type does not match pointer operand type!", &SI, ElTy);
+  }
   Assert(SI.getAlignment() <= Value::MaximumAlignment,
          "huge alignment values are unsupported", &SI);
   Assert(ElTy->isSized(), "storing unsized types is not allowed", &SI);
   if (SI.isAtomic()) {
     Assert(SI.getOrdering() != AtomicOrdering::Acquire &&
                SI.getOrdering() != AtomicOrdering::AcquireRelease,
            "Store cannot have Acquire ordering", &SI);
     Assert(SI.getAlignment() != 0,

llvm/test/Assembler/opaque-ptr.ll

 
 ; CHECK: define ptr addrspace(2) @g2(ptr %a) {
 ; CHECK:     %b = addrspacecast ptr %a to ptr addrspace(2)
 ; CHECK:     ret ptr addrspace(2) %b
 define ptr addrspace(2) @g2(ptr addrspace(0) %a) {
     %b = addrspacecast ptr addrspace(0) %a to ptr addrspace(2)
     ret ptr addrspace(2) %b
 }
+
+; CHECK: define i32 @load(ptr %a)
+; CHECK:     %i = load i32, ptr %a
+; CHECK:     ret i32 %i
+define i32 @load(ptr %a) {
+    %i = load i32, ptr %a
+    ret i32 %i
+}
+
+; 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
+}

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