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

[Sidharth Baveja via Phabricator via llvm-commits]

sidbav marked 4 inline comments as done.
sidbav 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,
----------------
asbirlea wrote:
> 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.
> ```
Updated comments to include a comment regarding L. 
Updated function name to be more verbose, and updated comments so the last sentences makes more sense. 


================
Comment at: llvm/lib/Transforms/Utils/BasicBlockUtils.cpp:330
+      if (MergeBlockIntoPredecessor(Dest, DTU, LI)) {
+        assert(!UniquePredecessor ||
+               (Fold == BB &&
----------------
asbirlea wrote:
> I don't think the `UniquePredecessor` argument is needed. The method `MergeBlockIntoPredecessor` will not merge unless `Dest` has a unique predecessor.
Yes that is a good point, and the code has been updated to reflect this change. 


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

https://reviews.llvm.org/D80583