[PATCH] D116970: a better profi rebalancer

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

Thanks for the work. Nice to see  0.5% perf win on clang-10 and gcc-8!



================
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;
 
----------------
How did this value come up? Have you tried other values between 20 and 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;
----------------
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?


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:565
+        HasUnknownSink = true;
+        continue;
+      }
----------------
return false here if DstBlock != nullptr ?


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:572
+      }
+      if (NumIgnoredJumps > 0 && NumIgnoredJumps == Block->SuccJumps.size()) {
         return false;
----------------
The check NumIgnoredJumps > 0 isn't necessary since Block->SuccJumps must not be empty here.


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:585
+  /// Decide whether the Jump is ignored while processing unknown subgraphs.
+  bool ignoreJump(const FlowBlock *SrcBlock, const FlowBlock *DstBlock,
+                  const FlowJump *Jump) {
----------------
Please add omment for SrcBlock and DstBlock.


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:618
+        continue;
       LocalInDegree[Jump->Target]++;
     }
----------------
nit: more compact without using continue:

```
if (!ignoreJump(SrcBlock, DstBlock, Jump))
   LocalInDegree[Jump->Target]++;
```


================
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
----------------
Can SrcBlock have unknown weight? It should have been filtered out by `canRebalanceAtRoot`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116970