[PATCH] D37343: [CGP] Merge empty case blocks if no extra moves are added.

[Balaram Makam via Phabricator via llvm-commits]

bmakam added a comment.

In https://reviews.llvm.org/D37343#875637, @efriedma wrote:

> I think this needs more testcases to illustrate the different possibilities here.


Thanks for taking a look Eli. I'll add more testcases.



================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:737
+  Function &F = *BB->getParent();
+  DominatorTree DT(F);
   // Do not delete loop preheaders if doing so would create a critical edge.
----------------
efriedma wrote:
> If you really need domtree, at least hoist computing it into it out into eliminateMostlyEmptyBlocks.  (Computing it here leads to quadratic complexity for large functions.)
DT gets modified whenever an empty block is eliminated so I had it here. I agree this is not efficient, let me see if I can hoist it out safely.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:784
+       auto *DestPN = dyn_cast<PHINode>(&*DestBBI); ++DestBBI) {
+    if (HasAllUsesDominatedByPred && !AllUsesDominatedByBlock(DestPN, Pred, DT))
+      HasAllUsesDominatedByPred = false;
----------------
efriedma wrote:
> Is this predicate significantly different from `dominates(Pred, DestBB)`?  I mean, in theory it's slightly different, but the difference isn't relevant in your testcase.
I'll add a testcase and hopefully it will be easier to understand.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:816
+      })) {
+    dbgs()<<DestBB->getName()<<"\n";
+    return true;
----------------
efriedma wrote:
> Leftover debug dump?
Oops! I will fix this.


================
Comment at: test/Transforms/CodeGenPrepare/skip-merging-case-block.ll:26
+  %v7 = or i64 %a1, 2251799813685248
+  br i1 undef, label %b8, label %b10
+
----------------
efriedma wrote:
> Please try to avoid undef in testcases.
I will fix this.


https://reviews.llvm.org/D37343