[PATCH] D102450: [OpaquePtr] Make loads and stores work with opaque pointers

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:3849
                  "type of pointer operand");
-  if (!PointerType::isLoadableOrStorableType(ElemType))
-    return error("Cannot load/store from pointer");
+  if (!ValType->isFirstClassType())
+    return error("Load/Store operand must be a pointer to a first class type");
----------------
dblaikie wrote:
> aeubanks wrote:
> > dblaikie wrote:
> > > 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?
> > 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
> 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)
made this closer to how it was originally


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102450/new/

https://reviews.llvm.org/D102450