[PATCH] D118640: [CSSPGO] Even count distribution
Hongtao Yu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Feb 13 23:41:29 PST 2022
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
More information about the llvm-commits
mailing list