[PATCH] D116970: a better profi rebalancer

[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:147
   /// A cost of taking an unlikely jump.
-  static constexpr int64_t AuxCostUnlikely = ((int64_t)1) << 20;
+  static constexpr int64_t AuxCostUnlikely = ((int64_t)1) << 30;
 
----------------
hoy wrote:
> How did this value come up? Have you tried other values between 20 and 30?
This is supposed to be an "infinity" value. I noticed however that 2^20 isn't large enough for some extreme cases (huge CFGs with thousand of basic blocks and very hot loops). Hence, increasing the value to 2^30.


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:555
+    // If there are multiple known sinks, we can't rebalance
+    if (KnownDstBlocks.size() > 1)
       return false;
----------------
hoy wrote:
> Can this be relaxed? Say if the exit block of a subgraph has unknown weight, the block may never be rebalanced. Eg.
> 
> ```
>        A
>     /     \
> B(?)     C(?)
>   |         |
>  D        E
>    \      /
>       F (?)
> ```
> 
> Did I understanding correctly?
I am not sure I understand this particular example, what needs to be rebalanced here? If A, D, and E have known weights, then a solution is unique, no?

But in general you're right that instances with `KnownDstBlocks.size() > 1` won't be rebalanced. We thought about this for a long time but the problem is difficult.


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:675
       uint64_t TotalFlow = 0;
       if (Block == SrcBlock) {
+        // SrcBlock's flow is the sum of outgoing flows along non-ignored jumps
----------------
hoy wrote:
> Can SrcBlock have unknown weight? It should have been filtered out by `canRebalanceAtRoot`.
good point. The reason is UnknownBlocks contains SrcBlock in this function.

Let me try to reimplement this part or apply some renaming to avoid confusion


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116970