[PATCH] D116970: a better profi rebalancer

[Sergey Pupyrev via Phabricator via llvm-commits]

spupyrev added inline comments.


================
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:
> spupyrev wrote:
> > 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.
> > If A, D, and E have known weights, then a solution is unique, no?
> 
> Say if we are rebalancing between `B` and `C`, due to this check and the fact `KnownDstBlocks` will include `D` and `E`, it'll bail out here?
> 
> > 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.
> 
> Can blocks in `KnownDstBlocks` be wired to a single artificial block which serves as the final only known dst? This is just my random thought. Hopefully the multi known case isn't too common.
Discussed this example offline. It seems we can slightly relax the condition here, but that only affects a single instance in clang binary and no instances in gcc binary. However, the adjustment makes the algorithm quadratic. Thus, keeping it as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116970