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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:720
+      DT->changeImmediateDominator(DestBB, NewIDom);
+      DT->eraseNode(BB);
+    }
----------------
bmakam wrote:
> 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.
FWIW: One of our usual concerns is that what people think is the right way to change the dominator tree is often incorrect in practice.
As for the niceness or not niceness, one significant advantage is that this can update the postdominatortree as well.

What you see as "change a simple dominator", even assuming it is right all the time for dominators, is definitely not right for postdominators :)

Let me tackle the inefficiency and verbosity:

Verbosity is something i mentioned to Jakub, and we're thinking about the kind of Updater interface that hides a little more of this (and updates both dom and postdom).

So i guess i'd much prefer to use the thing you think is inefficient because we know it is right, unless the inefficiency actually 
1. is real
2. matters.

(but we are completely aware the interface is a little verbose at the moment, and we're gathering data from how it's being used to know what to do to fix it)


https://reviews.llvm.org/D37343