[PATCH] D118694: [Bitcode] Add partial support for opaque pointer auto-upgrade

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1185-1190
+  if (Context.supportsTypedPointers()) {
+    Type *Ty = TypeList[ID];
+    if (Ty->isPointerTy())
+      return Ty->getNonOpaquePointerElementType();
+    return nullptr;
+  }
----------------
Would it be reasonable/possible to always use the `ElementTypeList` even when typed pointers are supported? (frontload changes somewhat) - maybe with typed pointer support this could be an assert that the type in `ElementTypeList` matches the non-opaque pointer type?

(oh, I guess since this support is currently incomplete that's not possible, but would be possible when the support is complete but before opaque pointers are enabled by default? I guess it could still be an assert today in the case where `ElementTypeList` is available, maybe?)


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5036-5037
+        Ty = getElementTypeByID(Record[0]);
+        if (!Ty)
+          return error("Missing element type for old-style alloca");
       }
----------------
I wonder if these sort of checks could be asserts? I realize they are dynamically reachable, but they're also not intended to be reached by end users (only by LLVM developers during this migration)... I guess it's weird either way. Either we have asserts that are dynamically reachable, or we have error paths that are untested... I don't feel great about either of those, but either seem like acceptable tradeoffs during the transition.


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

https://reviews.llvm.org/D118694