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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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");
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102450