[PATCH] D64235: [Loop Peeling] Fix the handling of branch weights of peeled off branches

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollPeel.cpp:367
 /// iteration correctly.
-/// Our goal is to make sure that:
-/// a) The total weight of all the copies of the loop body is preserved.
-/// b) The total weight of the loop exit is preserved.
-/// c) The body weight is reasonably distributed between the peeled iterations.
+/// Let F is a number of dynamic iteration to go to header from latch.
+/// Let E is a number of dynamic iteration to go to exit from latch.
----------------
Wording here is confusing.  I can infer the scheme from the rest of the comment, but I don't know what "dynamic iteration" means here.

I think you mean that F/(F+E) is the frequency with which we take the backedge, and that E/(F+E) is the frequency with which we exit the loop.  


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollPeel.cpp:419
 /// \param LatchBR The latch branch.
-/// \param ExitWeight The weight of the edge from Latch to Exit block.
-/// \param CurHeaderWeight The # of time the header is executed.
+/// \param ExitWeight The # of times the edge from Latch to Exit is taken.
+/// \param FallThroughWeight The # of times the edge from Latch to Header is
----------------
branch_weights are *frequencies* not *# of times executed*.

i.e. 1/2 is the same as 500/1000 and should always produce the same results.

The code appears to do so, but the comments are slightly misleading.  


================
Comment at: llvm/lib/Transforms/Utils/LoopUnrollPeel.cpp:426
+  // Sets the branch weights on the loop exit.
+  if (FallThroughWeight) {
     MDBuilder MDB(LatchBR->getContext());
----------------
Style: Use early return.  The existing code gets this wrong, so I'd encourage an NFC which changes that, and then a rebase.  It somewhat matters here as the case where FallThroughWeight is 0 is interesting, and deserves a comment and/or an assert.  


================
Comment at: llvm/test/Transforms/LoopUnroll/peel-loop-pgo-deopt.ll:76
 !17 = !{!"branch_weights", i32 1, i32 0}
 
+;CHECK: !16 = !{!"branch_weights", i32 3001, i32 1001}
----------------
If I'm reading this test correctly, the result is not making sense.

We have a loop (for.body) with a side exit branch frequency of 100% (%17) and a latch frequency of ~3 (%16).  

If we peel this twice, we should still have a very rarely taken side exit, but the frequencies appear to be saving that's more common in the peeled copy?  (The test naming is hard to follow, so I might just be reading the test wrong.)


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

https://reviews.llvm.org/D64235