[PATCH] D124878: [Bitcode] Include indirect users of BlockAddresses in bitcode

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Thanks for the patch! Indeed, the `users` _might_ be `Constant`s rather than just `Instructions`.  One thing to think about is that you can declare global `Constants`. I used the `dyn_cast<Instruction>` to check that this was not the case. I think you'll need to differentiate that, too. Meaning, if the user is a Constant, are the Constant's users Instructions (or not if simply GlobalVar).



================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3405-3408
-        if (auto *I = dyn_cast<Instruction>(U)) {
-          Function *P = I->getParent()->getParent();
-          if (P != &F)
-            BlockAddressUsers.insert(P);
----------------
Can we simply add additional `dyn_cast` here for `ConstantData`, `ConstantAggregate`, and `ConstantExpr` rather than two new data structures and loops?

That said, I guess Constant's are unique to a module, so not revisiting them is nice.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:3416-3417
+        for (User *U : V->users()) {
+          if ((isa<ConstantData>(U) || isa<ConstantAggregate>(U) ||
+               isa<ConstantExpr>(U)) &&
+              !BlockAddressUsersVisited.contains(U)) {
----------------
Consider adding test coverage for 3 different types of Users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124878