[PATCH] D127729: [Bitcode] Support expanding constant expressions into instructions

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

Most of this looks good to me, I just have a few nits and one bigger question about `phi` handling inline.



================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:99
+    "expand-constant-exprs", cl::init(false), cl::Hidden,
+    cl::desc("Expand constant expressions for testing purposes"));
+
----------------
Suggest to make it easier to understand for somebody who lacks context.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:520-524
+  uint8_t Opcode;
+  uint8_t Flags;
+  unsigned NumOperands;
+  unsigned Extra;  // GEP inrange index or blockaddress BB id.
+  Type *SrcElemTy; // GEP source element type.
----------------
Shouldn't these be `private`?


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1421-1425
+      if (It == MaterializedValues.end()) {
+        Worklist.push_back(OpID);
+        break;
+      }
+      Ops.push_back(It->second);
----------------
Could be an if/else without a `break` so that all required values are pushed to the worklist immediately, which is slightly more convenient for the common case where the constant expression overall is a tree. But I don't feel strongly about it.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5569-5582
+        if (!PhiConstExprBB->empty()) {
+          auto It = Args.find(BB);
+          if (It != Args.end()) {
+            // If this phi argument has already been expanded, reuse the
+            // previous value instead of materializing a new one.
+            V = It->second;
+            PhiConstExprBB->eraseFromParent();
----------------
I don't understand what `Args` is trying to achieve here. Each predecessor basic block should only appear once, so the `Args.find(BB)` should never be successful -- right?

I *think* perhaps what you're trying to do is optimize the case where multiple `phi` nodes have the same incoming value for the same basic block? But then the `Args` map would have to have bigger scope and look slightly different.

Speaking of, shouldn't the PhiConstExprBB be cached somehow so that multiple `phi` nodes use the same edge basic blocks?


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

https://reviews.llvm.org/D127729