[PATCH] D25963: [LoopUnroll] Implement profile-based loop peeling

[David Li via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopUnrollPass.cpp:724
     unsigned MaxTripCount, unsigned &TripMultiple, unsigned LoopSize,
-    TargetTransformInfo::UnrollingPreferences &UP, bool &UseUpperBound) {
+    unsigned &PeelCount, TargetTransformInfo::UnrollingPreferences &UP,
+    bool &UseUpperBound) {
----------------
mkuper wrote:
> davidxl wrote:
> > Should PeelCount be a member of UnrollingPreferences class?
> I think not, but I don't really know.
> To the best of my understanding, the main point of UnrollingPreferences is to give targets the ability to override the defaults, and I'm not sure this is useful here.
The Count field in UP is used to specify unrolling factor including heuristic based :

 // 4rd priority is partial unrolling.
  // Try partial unroll only when TripCount could be staticaly calculated.
  if (TripCount) {
    if (UP.Count == 0)
      UP.Count = TripCount;


Conceptually peeling Count should be treated in the same way.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:87
+    VMap[*BB] = NewBB;
+    if (Header == *BB)
+      InsertTop->getTerminator()->setSuccessor(0, NewBB);
----------------
Can this be moved outside the loop?

assert(VMap[Header]);
InsertTop->getTerminator()->setSuccessor(0, VMap[Header]);


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:90
+
+    if (Latch == *BB) {
+      VMap.erase((*BB)->getTerminator());
----------------
This can be handled outside the loop too.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:91
+    if (Latch == *BB) {
+      VMap.erase((*BB)->getTerminator());
+      BranchInst *LatchBR = cast<BranchInst>(NewBB->getTerminator());
----------------
What this this erase do?


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:95
+      // If this is the last iteration, branch past the loop, otherwise
+      // branch to the next iteration (which may itself either be peeled off,
+      // or the loop's preheader)
----------------
Is this a stale comment?


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:101
+      // We no longer know anything about the branch probability.
+      LatchBR->setMetadata(LLVMContext::MD_prof, nullptr);
+    }
----------------
Why? I think we should update the branch probability here -- it depends on the what iteration of the peeled clone. If peel count < average/estimated trip count, then each peeled iteration should be more biased towards fall through. If peel_count == est trip_count, then the last peel iteration should be biased toward exit.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:220
+  }
+
+  // Now adjust the phi nodes in the loop header to get their initial values
----------------
The profile meta data of the original loop's back branch should be adjusted too.


https://reviews.llvm.org/D25963