[PATCH] D28593: Update loop branch_weight metadata after loop rotation.

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:84
+  /// up underestimating the tripcount each time the loop is executed.
+  void updateLoopEstimatedBranchWeight(Loop *L, BranchInst *, BranchInst *);
 };
----------------
name the parameters.


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:481
+  // Adjust the loop branch metadata in case we have them.
+  updateLoopEstimatedBranchWeight(L, GBI, BI /* OrigHeader BR */);
+
----------------
Remove the comment about OrigHeader BR. After Block merging, it will become the latch block branch instruction.

Is it better to call this after MergeBlockInfoPredecessor when loop L is a more consistent state?


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:503
+                                                 BranchInst *LatchBR) {
+  // If loop has multiple exitting block, we need to look through each one
+  // of them and reason about their profile metadata to compute the rotated
----------------
exitting --> exiting


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:528
+    if (LoopBodyWeight >= LoopExitWeight) {
+      // In case profiled LoopBodyWeight >= LoopExitWeight, we assume the
+      // branch-to-exit in the guard branch is never taken, as on average the
----------------
you need a test case to cover LoopBodyWeight > LoopExitWeight case  --- not the one with known tripcount. For the constant trip count case, the guard BR gets eliminated.


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:567
+      GuardBR->setMetadata(LLVMContext::MD_prof,
+                           GuardBR->getSuccessor(0) == L->getLoopPreheader() ?
+                           MDB.createBranchWeights(LoopExitWeight,
----------------
Explain this condition?  Perhaps a test case?


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:570
+                                                   NotTakeOnceWeight) :
+                           MDB.createBranchWeights(NotTakeOnceWeight,
+                                                   LoopExitWeight));
----------------
This is probably not quite correct.

For the case when LoopBodyWeight > LoopExitWeight, especially when the runtime trip count is large, the guard BR should have very biased against exiting. I think it is reasonable to use an arbitrary biased exit probability such as 1/100.

For cases when LoopBodyWeight and LoopExitWeight are close (e.g., trip count < 5), it is reasonable to keep GuardBR has the same exit branch probability as the new LatchBR. This applies to LoopBodyWeight < LoopExitWeight case as well.


================
Comment at: test/Transforms/LoopRotate/loop-rotate-pgo.ll:9
+; rotation.
+define i32 @loops_with_big_tripcount() {
+entry:
----------------
Perhaps name it 'loop_with_known_trip_count'


================
Comment at: test/Transforms/LoopRotate/loop-rotate-pgo.ll:16
+   %cmp = icmp slt i32 %index, 1024
+   br i1 %cmp, label %for.body, label %for.end, !prof !0
+
----------------
This loop has statically known trip count. The profile data does not match it. Use a different meta data.


================
Comment at: test/Transforms/LoopRotate/loop-rotate-pgo.ll:32
+
+; This loop has a small trip count, most of the time it simply branches
+; to for.end. 
----------------
add comment the loop body is by-passed most of the time.


================
Comment at: test/Transforms/LoopRotate/loop-rotate-pgo.ll:36
+; CHECK: icmp slt i32 15, %a
+; CHECK-NEXT: !prof !0 
+entry:
----------------
explicitly check br instruction as well  so that two branch targets are listed.  !prof is meaningless without looking at the branch


================
Comment at: test/Transforms/LoopRotate/loop-rotate-pgo.ll:50
+; CHECK: icmp slt i32 %inc, %a
+; CHECK-NEXT: !prof !1
+for.inc:
----------------
Same here -- also check branch instruction


https://reviews.llvm.org/D28593