[PATCH] D82927: Intergerate Loop Peeling into Loop Fusion

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:124
+
+static cl::opt<unsigned> FusionPeelMaxCount(
+    "loop-fusion-peel-max-count", cl::init(3), cl::Hidden,
----------------
We only really need one option to control peeling for loop fusion. You can remove `loop-fusion-peel` and change the default for this threshold to 0. I don't know what others feel about this, but I'd rather have shorter commands when possible. 


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:758
+
+    LLVM_DEBUG(dbgs() << "Difference in Loop trip count is: " << Difference
+                      << "\n");
----------------
nit: Loop -> loop


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:766
+                           unsigned PeelCount) {
+    assert(FC0.AbleToPeel && "Should be able to Peel Loop");
+
----------------
nit: Peel Loop -> peel loop.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:814
+      LLVM_DEBUG(
+          dbgs() << "Sucessfully Peeled " << FC0.PP.PeelCount
+                 << " iterations from the first loop.\n"
----------------
nit: Peeled -> peeled


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:861
+
+          // Check if the user specfices to use Peeling with fusion
+          // Check FC0 (first loop) is able to be peeled
----------------
nit: comments should be full sentences ending with a period.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:862
+          // Check if the user specfices to use Peeling with fusion
+          // Check FC0 (first loop) is able to be peeled
+          // Check that both loops have the different tripcounts
----------------
is able to be peeled -> can be peeled


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:863
+          // Check FC0 (first loop) is able to be peeled
+          // Check that both loops have the different tripcounts
+          if (FusionPeel.getNumOccurrences() > 0 && FusionPeel &&
----------------
the different -> different


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:867
+            assert(*TCDifference <= FusionPeelMaxCount &&
+                   "Peel Count: is greater than maximum value specificed");
+            // Dependent on peeling being performed on the first loop, and
----------------
Please change this to give up on such loops instead of asserting.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1252
 
   /// Simplify the condition of the latch branch of \p FC to true, when both of
   /// its successors are the same.
----------------
This function no longer does what this comment says.  Please update the comment.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1558
+    // of the FC0 Exit block to the to the beginning of the exit block of FC1.
+    if (!FC0.Peeled)
+      moveInstructionsToTheBeginning(*FC0.ExitBlock, *FC1.ExitBlock, DT, PDT,
----------------
`moveInstructionsToTheBeginning(FC0.Peeled ? *FC0ExitBlockSuccessor : *FC0.ExitBlock, *FC1.ExitBlock, DT, PDT, DI);`


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1583
     FC0.GuardBranch->replaceUsesOfWith(FC0NonLoopBlock, FC1NonLoopBlock);
-    FC0.ExitBlock->getTerminator()->replaceUsesOfWith(FC1GuardBlock,
-                                                      FC1.Header);
+    if (!FC0.Peeled)
+      FC0.ExitBlock->getTerminator()->replaceUsesOfWith(FC1GuardBlock,
----------------
```
BasicBlock *BBToUpdate = FC0.Peeled ? FC0ExitBlockSuccessor : FC0.ExitBlock;
BBToUpdate->getTerminator()->replaceUsesOfWith(FC1GuardBlock, FC1.Header);
```


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

https://reviews.llvm.org/D82927