[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:559
+    // equal to 1). We give it a 50% chance to exit.
+    double NotTakenOnceRatio = HasLargeAverageTripCount ? 0.05 : 0.5;
+
----------------
What if the average trip count is > 20? In that case, 0.05 is larger than the latch br exit probability which is not right.  The 'min' needs to be taken.

It is also better to use fixed point operation (with BranchProbablilty and BlockFrequency classes)

BranchProbability GuardBP(5, 100);
uint64_t NotTakenOnceWeight = (BlockFrequency(LoopExitWeight) * GuardBP).getFrequency();
uint64_t LoopPHWeight =  LoopExitWeight - NotTakenOnceWeight;

GuardBR->setMetadata(MD_prof, ...);

'5' also needs to be  a parameter. Do not use hard coded number.




================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:591
+    // give the guard branch a 50% chance to exit.
+    const double NotTakenOnceRatio = HasSmallAverageTripCount ? 0.95 : 0.5;
+
----------------
same here, 0.95 may be smaller than the original Header BR's exit branch probability. The max should be taken.

The value 0.5 is not necessary -- why not just directly use original header BR's exit BP?


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:604
+    // a single iteration of the loop.
+    NotTakenOnceWeight = std::max(NotTakenOnceWeight, LoopExitWeight - LoopBodyWeight);
+    // # of times loop is entered is the same as # of times the latch branch
----------------
This can be done by computing the right notTakenOnceRatio above.


================
Comment at: lib/Transforms/Scalar/LoopRotation.cpp:617
+    NotTakenOnceWeight = std::max(NotTakenOnceWeight, MinimumEdgeWeight);
+    BackedgeWeight = std::max(BackedgeWeight, MinimumEdgeWeight);
+  }
----------------
The weight computation code above should be refactored and shared across different cases. The difference only lies in the way NotTakenOnceRatio is computed.


https://reviews.llvm.org/D28593