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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added subscribers: kuhar, dberlin.
efriedma added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:720
+      DT->changeImmediateDominator(DestBB, NewIDom);
+      DT->eraseNode(BB);
+    }
----------------
bmakam wrote:
> bmakam wrote:
> > efriedma wrote:
> > > New code should be using insertEdge/deleteEdge/applyUpdates to do domtree updates.
> > I found this to be more easy to understand, I am happy to change this to DominatorTree::UpdateTypes/applyUpdates if this is the new coding standard.
> I tried using the new batch updater interface for DominatorTree. I create a vector of updates. First, insert edges from all predecessors of BB to DestBB. Second, remove edges from all predecessors of BB to BB  and finally, delete edge from BB to DestBB. IIUC, the batch updater is efficient for multiple CFG updates, but in this case updating the IDom and erasing the Node seems more efficient to me.
> 
> ```
> +    eliminateMostlyEmptyBlock(BB);
>      if (DT && !ModifiedDT) {
> +/*
>        BasicBlock *BBIDom = DT->getNode(BB)->getIDom()->getBlock();
>        BasicBlock *DestBBIDom = DT->getNode(DestBB)->getIDom()->getBlock();
>        BasicBlock *NewIDom = DT->findNearestCommonDominator(BBIDom, DestBBIDom);
>        DT->changeImmediateDominator(DestBB, NewIDom);
>        DT->eraseNode(BB);
> +*/
> +      SmallVector<DominatorTree::UpdateType, 3> Updates;
> +      for (auto *PredBB : predecessors(BB)) {
> +        if (PredBB == BB)
> +          continue;
> +        DominatorTree::UpdateType UT = {DominatorTree::Insert, PredBB, DestBB};
> +        if (find(Updates, UT) == Updates.end()) {
> +          Updates.push_back(UT);
> +          Updates.push_back({DominatorTree::Delete, PredBB, BB});
> +        }
> +      }
> +      if (!Updates.empty()) {
> +        Updates.push_back({DominatorTree::Delete, BB, DestBB});
> +        DT->applyUpdates(Updates);
> +      }
>      }
> -    eliminateMostlyEmptyBlock(BB);
> ```
> What do you think?
IIRC some later dom operations become more expensive after you use changeImmediateDominator, but I don't remember the details.  Adding a couple people to the CC list who would know more.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:349
+      getAnalysisIfAvailable<DominatorTreeWrapperPass>();
+  DT = DTWP ? &DTWP->getDomTree() : nullptr;
+
----------------
You should be able to guarantee DT is available by putting "AU.addRequired<DominatorTreeWrapperPass>();" in getAnalysisUsage().  That will simplify a bunch of null checks.  (It's okay not to preserve it for now, but add a comment noting that you're intentionally not preserving it.)


https://reviews.llvm.org/D37343