[PATCH] D123050: [BOLT] Cache-Aware Tail Duplication

[Rafael Auler via Phabricator via llvm-commits]

rafauler accepted this revision.
rafauler added a comment.
This revision is now accepted and ready to land.

LGTM with a few comments below



================
Comment at: bolt/lib/Passes/TailDuplication.cpp:393
+  if (IsForwardJump) {
+    JumpDistance = (DstAddr + DstSize) - (SrcAddr);
+  } else {
----------------
By reading "jump distance" I would expect to see
  JumpDistance = DstAddr - (SrcAddr + SrcSize)
as an approximation of the jump distance in a forward jump.  E.g.
  BlockA:  <src address>
  nop
  nop
  jmp BlockB 
  BlockB:  <src address + src size>     also  <dstaddr>
  nop
  nop
  nop
  <dstaddr + dst size>

In the example above, because jmp BlockB is a fall-through, it should calculate 0 as the distance. Using the definition I provided above, it is zero, but not in the source code. In the source code of this diff, JumpDistance would be "src size + dst size".  Is this to calculate how much cache space is used by using these two blocks? What's the idea? Can you make more explicit the reasoning in the comments? From reading the comments, I got the impression that a FT should be calculated as zero distance.


================
Comment at: bolt/lib/Passes/TailDuplication.cpp:402
+                          opts::TailDuplicationMaxCacheDistance;
+  return (IsForwardJump ? 1.0 : opts::TailDuplicationCacheBackwardWeight) *
+         Prob * Count;
----------------
Why are we always returning 1.0 in case of a forward jump? Is this correct? From what I read in the comments of this function, I would expect to see Prob * Count here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123050