[PATCH] D80583: Create utility function to Merge Adjacent Basic Blocks

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/BasicBlockUtils.h:106
+/// return true.
+bool MergeAdjacentBlocks(SmallPtrSetImpl<BasicBlock *> &MergeBlocks,
+                         bool UniquePredecessor = true, Loop *L = nullptr,
----------------
Include a comment that if L is given, the blocks merged into their predecessors must be in L.


A pretty verbose renaming suggestion: `MergeBlockSuccessorsIntoGivenBlocks`, since this is not merging any adjacent blocks.


If the intent was "if L not given, then MergeBlocks.contains(Dest)", include that in code; this is not a usecase in unrollAndJam, so I cannot tell, but reading the method comment and name this is what I'd assume the method does. Or, rename to something more specific than "adjacentBlocks".


I do not understand this comment: 
```
In addition, a call to MergeBlockIntoPredecessor where
/// the block following the terminal branch is passed must
/// return true.
```


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:330
+      if (MergeBlockIntoPredecessor(Dest, DTU, LI)) {
+        assert(!UniquePredecessor ||
+               (Fold == BB &&
----------------
I don't think the `UniquePredecessor` argument is needed. The method `MergeBlockIntoPredecessor` will not merge unless `Dest` has a unique predecessor.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80583/new/

https://reviews.llvm.org/D80583