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

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

nhaehnle added a comment.

Thanks. This looks pretty good to me, but I'm not familiar with the bitcode reader so perhaps somebody else can still take a look.



================
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();
----------------
nikic wrote:
> 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).
Huh, I didn't realize duplicate predecessors were allowed. Weird, but okay :)


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

https://reviews.llvm.org/D127729