[PATCH] D48202: Generalize MergeBlockIntoPredecessor. Replace uses of MergeBasicBlockIntoOnlyPred.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

Thank you for the comments!



================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:527-528
+    BasicBlock *BB = cast_or_null<BasicBlock>(Block);
+    if (!BB)
+      continue;
     // If the destination block has a single pred, then this is a trivial
----------------
dberris wrote:
> You could reverse this, so you end up with:
> 
> ```
> for (auto &Block : Blocks) {
>   if (auto *BB = cast_or_null<BasicBlock>(Block)) {
>     ...
>   }
> }
> ```
Agreed, but I favored reducing nesting here for easier readability.


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:146
   // Can't merge if there are multiple successors.
   if (!OnlySucc) return false;
 
----------------
dmgreen wrote:
> This and the previous block is just (PredBB->getUniqueSuccessor() == BB)?
This method is expected to also merge blocks when there are multiple edges from PredBB to BB.
Example: `test/Transforms/SimplifyCFG/2003-08-17-FoldSwitch.ll: test3`

```
Start:          ; preds = %0
  switch i32 3, label %TheDest [
  i32 0, label %TheDest
  i32 1, label %TheDest
  i32 2, label %TheDest
  i32 5, label %TheDest
  ]
TheDest:
  ret i32 1234
```


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:169
+    Updates.reserve(1 + (2 * succ_size(BB)));
+    Updates.push_back({DominatorTree::Delete, PredBB, BB});
+    for (auto I = succ_begin(BB), E = succ_end(BB); I != E; ++I) {
----------------
dberris wrote:
> Consider `Updates.emplace_back(DominatorTree::Delete, PredBB, BB)` instead? Or does that not work because DominatorTree::UpdateType is an aggregate?
Being an aggregate should still work, but it doesn't link because it can't find Delete, declared as static constexpr in DominatorTreeBase. The options I got are:
- Instead of DominatorTree::Delete use the "original" enum which is DomTreeBuilder::UpdateKind::Delete, but that's icky to expose.
- Use a temporary local variable `auto tmp = DominatorTree::Delete`, and use that in the emplace_back.
- Use push_back.
I'm inclined to just keep the push_back here, but let me know if you think otherwise.


================
Comment at: lib/Transforms/Utils/BasicBlockUtils.cpp:173
+      // This successor of BB may already have PredBB as a predecessor.
+      if (llvm::find(predecessors(*I), PredBB) == pred_end(*I))
+        Updates.push_back({DominatorTree::Insert, PredBB, *I});
----------------
dmgreen wrote:
> I see this comes from the other function, but PredBB should only have one successor (BB), if I am reading this correctly. So this should always be true.
Agreed, updated.


Repository:
  rL LLVM

https://reviews.llvm.org/D48202