[PATCH] D139870: [BOLT] using jump weights in profi

[Sergey Pupyrev via Phabricator via llvm-commits]

spupyrev marked 2 inline comments as done.
spupyrev added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:1088
     // Edges from S and to T
-    assert((!Block.isEntry() || !Block.isExit()) &&
-           "a block cannot be an entry and an exit");
----------------
hoy wrote:
> Does this still hold?
the assert is moved to `verifyInput`


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:1120
-    // as all of the increase can be attributed to the self-edge
-    if (Block.HasSelfEdge) {
-      AuxCostDec = 0;
----------------
hoy wrote:
> Where is this handled in the new implementation?
This is a good question; we don't have this condition anymore. While in theory there might be a difference, I do not see a single instance (in my benchmark with 10K functions) where this statement yields a different result. So it must be quite rare.
More importantly, I do not remember the original motivation: Why is it "correct" not to penalize blocks with self loops?


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:1125
-    Network.addEdge(Bin, Baux, AuxCostInc);
-    Network.addEdge(Baux, Bout, AuxCostInc);
     if (Block.Weight > 0) {
----------------
hoy wrote:
> Previously the cost it takes to go from `Bin` to `Bout` is `2*AuxCostInc`. Now without `Baux` the cost for the same path becomes `AuxCostInc`. Is it a discrepancy or am I missing anything? 
Now all the costs are reduced by a factor of 2, that is, instead of `2*AuxCostInc` we have `AuxCostInc` and instead of `2*AuxCostDec` we have `AuxCostDec`. This does not have any impact on the algorithm and the produced solution; only the objective is (uniformly) scaled


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139870