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

[Balaram Makam via Phabricator via llvm-commits]

bmakam added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:720
+      DT->changeImmediateDominator(DestBB, NewIDom);
+      DT->eraseNode(BB);
+    }
----------------
efriedma wrote:
> 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.
Interesting. Thanks, Eli. Anyways, I have now updated the patch to not use changeImmediateDominator and use the new batch update interfaces.


https://reviews.llvm.org/D37343