[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

7537–7539

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

3852

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

3759

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

3852

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

3852

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

3852

made this closer to how it was originally

[dblaikie]

Sounds good!

llvm/include/llvm/IR/DerivedTypes.h

public:
   static bool isValidElementType(Type *ElemTy);
 
   /// Return true if we can load or store from a pointer to this type.
   static bool isLoadableOrStorableType(Type *ElemTy);
 
   /// Return the address space of the Pointer type.
   inline unsigned getAddressSpace() const { return getSubclassData(); }
 
+  /// Return true if either this is an opaque pointer type or if this pointee
+  /// type matches Ty. Primarily used for checking if an instruction's pointer
+  /// operands are valid types. Will be useless after non-opaque pointers are
+  /// removed.
+  bool isOpaqueOrPointeeTypeMatches(Type *Ty) {
+    return isOpaque() || PointeeTy == Ty;
+  }
+
   /// Implement support type inquiry through isa, cast, and dyn_cast.
   static bool classof(const Type *T) {
     return T->getTypeID() == PointerTyID;
   }
 };
 
 Type *Type::getExtendedType() const {
   assert(

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())->isOpaqueOrPointeeTypeMatches(Ty)) {
     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())
+           ->isOpaqueOrPointeeTypeMatches(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))
     return error(Loc, "storing unsized types is not allowed");

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

Error BitcodeReader::parseBitcodeInto(Module *M, bool ShouldLazyLoadMetadata,
   MDLoader = MetadataLoader(Stream, *M, ValueList, IsImporting,
                             [&](unsigned ID) { return getTypeByID(ID); });
   return parseModule(0, ShouldLazyLoadMetadata, DataLayoutCallback);
 }
 
 Error BitcodeReader::typeCheckLoadStoreInst(Type *ValType, Type *PtrType) {
   if (!isa<PointerType>(PtrType))
     return error("Load/Store operand is not a pointer type");
-  Type *ElemType = cast<PointerType>(PtrType)->getElementType();
 
-  if (ValType && ValType != ElemType)
+  if (!cast<PointerType>(PtrType)->isOpaqueOrPointeeTypeMatches(ValType))
     return error("Explicit load/store type does not match pointee "
                  "type of pointer operand");
-  if (!PointerType::isLoadableOrStorableType(ElemType))
+  if (!PointerType::isLoadableOrStorableType(ValType))
     return error("Cannot load/store from pointer");
   return Error::success();
 }
 
 void BitcodeReader::propagateByValSRetTypes(CallBase *CB,
                                             ArrayRef<Type *> ArgsFullTys) {
   for (unsigned i = 0; i != CB->arg_size(); ++i) {
     for (Attribute::AttrKind Kind : {Attribute::ByVal, Attribute::StructRet,
                                      Attribute::InAlloca}) {

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())->isOpaqueOrPointeeTypeMatches(Ty));
   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())->isOpaqueOrPointeeTypeMatches(Ty));
   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())
+             ->isOpaqueOrPointeeTypeMatches(getOperand(0)->getType()) &&
+         "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();
+  Assert(PTy->isOpaqueOrPointeeTypeMatches(ElTy),
          "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,
            "Atomic store must specify explicit alignment", &SI);

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