[PATCH] D118640: [CSSPGO] Even count distribution

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

Thanks for working on the smart algorithm and the offline illustration of the high-level idea. The performance win on clang-10 looks promising.



================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:197
+  /// saturated (that is, no flow can be sent along the path), then return 0.
+  uint64_t augmentingPathCapacity() {
+    uint64_t PathCapacity = INF;
----------------
nit: name it computeAugmentingPathCapacity?


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:301
+    //  - we are currently visiting Nodes[NodeIdx] and
+    //  - the next edge to scan is Edges[Nodes[NodeIdx]][EdgeIdx]
+    typedef std::pair<uint64_t, uint64_t> StackItemType;
----------------
nit: Edges[NodeIdx][EdgeIdx]?


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:333
+          // If we haven't seen Edge.Dst so far, continue DFS search there
+          if (Dst.Discovery == 0 && Dst.NumCalls < SampleProfileMaxDfsCalls) {
+            Dst.Discovery = ++Time;
----------------
Should the Discovery field be should a boolean? Looks like this is the only place using it.


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:333
+          // If we haven't seen Edge.Dst so far, continue DFS search there
+          if (Dst.Discovery == 0 && Dst.NumCalls < SampleProfileMaxDfsCalls) {
+            Dst.Discovery = ++Time;
----------------
hoy wrote:
> Should the Discovery field be should a boolean? Looks like this is the only place using it.
I was wondering if setting `SampleProfileMaxDfsCalls=1` makes sense. It looks like whether `Dst` is taken or not doesn't matter with which predecessor the searching starts with. Correct me if I'm wrong. 


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:335
+            Dst.Discovery = ++Time;
+            Stack.push(StackItemType(Edge.Dst, 0));
+            Dst.NumCalls++;
----------------
nit: Stack.emplace(Edge.Dst, 0)


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:354
+          if (NodeIdx != Source) {
+            Nodes[Stack.top().first].Taken = true;
+          }
----------------
assert stack must not be empty at this point?


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:398
+    for (uint64_t Src : AugmentingOrder) {
+      assert(Src == Target || Nodes[Src].FracFlow > 0.0);
+      // Distribute flow evenly among successors of Src
----------------
Please add a message for the assertion.


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:482
+    for (size_t Src = 0; Src < Nodes.size(); Src++) {
+      // An edge cannot be augmenting if the incident node has large distance
+      if (Nodes[Src].Distance > Nodes[Target].Distance)
----------------
What is an incident node?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118640