[PATCH] D24918: [ADCE] Add code to remove dead branches

[Mehdi AMINI via llvm-commits]

mehdi_amini added a comment.

Sorry for the delay, I keep getting interrupted in the middle of getting this code into my head, and paging out has a real cost when getting back to it. Especially as I don't really know the algorithm, I have to infer what you're doing from the implementation (which isn't a bad way to judge of the clarity of the code).

Anyway, a few inline comments.



================
Comment at: lib/Transforms/Scalar/ADCE.cpp:610
+  }
+
+  // Update each live phi based on the new incoming edges.
----------------
david2050 wrote:
> dberlin wrote:
> > Is the above really necessary?
> > 
> > The standard way to do this, AFAIK, is to mark the useful block set while doing marking, and then just walking up the PDT to find the nearest block marked useful for each dead branch.
> > Replace all uses of the bb with that block using RAUW.
> > This should update phi nodes, since I believe the blocks in phi nodes are still considered uses.
> >  
> I don't understand this approach. Given a live branch  (x,y) where y is dead and  z is the live post-dominator of y, we can't just replace uses of 'y' because the 'y' may not be directly referenced in phi nodes in 'z' which only will old references to its predecessors. Also, 'y' could have multiple branches into it which are dead.
I'm not sure I understood what @dberlin referred to, but as I read it it should be possible to just process DeadBlocks without computing the dead region, and only manipulate Phi in the live PDOM if the live PDOM is a direct successor of the current block. I think this should get the same result in term of Phi, but does not seems less costly than computing the region.
Also I'm not sure if you can rewrite the branches from the live predecessors into the dead region to jump directly to the live PDOM without computing the region (but with the invariant you are already relying on to get a dead region with a unique live PDOM it may be possible).



================
Comment at: lib/Transforms/Scalar/ADCE.cpp:240
+    for (auto *BB: depth_first_ext(&F.getEntryBlock(), State)) {
+      
+      TerminatorInst *Term = BB->getTerminator();
----------------
Remove this empty line?


================
Comment at: lib/Transforms/Scalar/ADCE.cpp:496
+  for (Instruction *I : NewBranches)
+    InstInfo[I].Live = true;
+
----------------
This is quite complex to follow: the `NewBranches` is some state that is kept on the side and updated by `makeUnconditional` (transitively called by `updateDeadRegions`), and used here as a cleanup. It is never cleared BTW.
Maybe declaring NewBranches locally to this function and passing it by as an output parameter to `updateDeadRegions()`?




================
Comment at: lib/Transforms/Scalar/ADCE.cpp:571
+
+  // Find an unprocsesed block which identifies a new dead region.
+  for (BasicBlock *BB : BlocksWithDeadTerminators) {
----------------
`s/unprocsesed/unprocessed/`


================
Comment at: lib/Transforms/Scalar/ADCE.cpp:580
+void AggressiveDeadCodeElimination::updateDeadRegions(
+    BasicBlock *BB, SmallPtrSet<BasicBlock *, 16> *VisitedBlocks) {
+
----------------
Parameter should be `SmallPtrSetImpl<BasicBlock *> *VisitedBlocks`.  The template with the size is only used where you need to actually create one.

(Also I'd use a reference instead of pointer)




================
Comment at: lib/Transforms/Scalar/ADCE.cpp:611
+    if (!BlockInfo[BB].Live) {
+      for (auto PredBB : predecessors(BB)) {
+        if (BlocksWithDeadTerminators.count(PredBB))
----------------
`auto *` for pointers
(Many other occurrences)


================
Comment at: lib/Transforms/Scalar/ADCE.cpp:621
+    // Scan for dead successors. A live successor will be the
+    // unique post-dominator for all of the blocks in this region.
+    for (auto SuccBB : successors(BB)) {
----------------
How do we get this invariant that a region has a single live PDOM?
(If you can point me to the relevant code)


================
Comment at: lib/Transforms/Scalar/ADCE.cpp:628
+      else
+        LivePDOM = SuccBB;
+    }
----------------
Could be compressed to a single else:

```
else  {
  assert(!LivePDOM || LivePDOM == SuccBB);
  LivePDOM = SuccBB;
}
```

Also we usually try to add a message with assertions when possible.


================
Comment at: lib/Transforms/Scalar/ADCE.cpp:665
+      // This is a live block but the terminator is not live so all
+      // control reaches the LivePdom, so we make the terminator
+      // an unconditional branch to that destination (B29 in the example
----------------
To make sure I understand correctly: if the terminator was a conditional branch with each target BB in different dead region, then this terminator shouldn't be marked as dead?


================
Comment at: lib/Transforms/Scalar/ADCE.cpp:667
+      // an unconditional branch to that destination (B29 in the example
+      // above).
+      makeUnconditional(PredBB, LivePDOM);
----------------
/me scrolling but don't see the example...


https://reviews.llvm.org/D24918