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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:953
+  BB->removeFromParent();
+  if (BB->getParent() == DestBB->getParent() && BB != DestBB)
+    DT->deleteEdge(BB, DestBB);
----------------
bmakam wrote:
> kuhar wrote:
> > bmakam wrote:
> > > bmakam wrote:
> > > > efriedma wrote:
> > > > > "BB->getParent() == DestBB->getParent()" still here.  What is that trying to check for?
> > > > err, I was trying to avoid hitting this assert from DT->deleteEdge:
> > > >  void llvm::DominatorTreeBase<llvm::BasicBlock, false>::deleteEdge(NodeT *, NodeT *) [NodeT = llvm::BasicBlock, IsPostDom = false]: Assertion `From->getParent() == Parent' failed.
> > > > 
> > > > But looks like this code is unreachable, so I don't need to delete the edge BB, DestBB after BB->eraseFromParent()?
> > > I think the right way to delete the edge is
> > > 1. BB->getTerminator()->eraseFromParent()
> > > 2. DT->deleteEdge(BB, DestBB)
> > > 3. BB->eraseFromParent()
> > > 
> > > This way I don't hit the assert and I can delete the edge in DT.
> > Are you sure that calling `BB->removeFromParent()` actually sets the BB's Parent pointer? The sequence of calls I mention is used in a couple of places quite some time now.
> > 
> > Your method looks correct as well, but I wouldn't call it the most intuitive one :).
> Its possible I might be missing something here, but if I call BB->removeFromParent() and then DT->deleteEdge(BB, DestBB) I hit the above assert for 533 lit test cases with ninja check.
Ah, I see, I missed that you have an edge BB -> DestBB and want to delete *BB*. In this case, your code looks fine.


https://reviews.llvm.org/D37343