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

[Nikita Popov via Phabricator via llvm-commits]

nikic marked an inline comment as done.
nikic added inline comments.


================
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.
----------------
nhaehnle wrote:
> Shouldn't these be `private`?
These are directly used in the materializeValue() code. It didn't seem worthwhile to make these private and add getters for something that is private to the bitcode reader anyway.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1421-1425
+      if (It == MaterializedValues.end()) {
+        Worklist.push_back(OpID);
+        break;
+      }
+      Ops.push_back(It->second);
----------------
nhaehnle wrote:
> 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.
Good point, done. A subtlety here is that we now need to iterate in reverse order (and then reverse Ops) to make sure the worklist entries are processed in correct order. Otherwise we'd expand the last operand first.


================
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();
----------------
nhaehnle wrote:
> 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?
A predecessor can appear multiple times in a phi block, but must have the same incoming value each time. This most commonly happens with switch terminators (see @test_phi_multiple_identical_predecessors). I've made the comment for this a bit more detailed.

And you're absolutely right, I wasn't handling the case of multiple phis correctly. I've changes this to reuse the same block for all expressions on one edge (@test_phi_multiple_nodes).


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

https://reviews.llvm.org/D127729