[PATCH] D147812: [InstrProf] Use BalancedPartitioning to order temporal profiling trace data
Sergey Pupyrev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue May 2 07:17:37 PDT 2023
spupyrev added inline comments.
Herald added a subscriber: hoy.
================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:147
+ unsigned NumNodes = std::distance(Nodes.begin(), Nodes.end());
+ if (NumNodes < 1 || RecDepth >= Config.SplitDepth) {
+ // We've reach the lowest level of the recursion tree. Fall back to the
----------------
================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:200
+ unsigned NumNodes = std::distance(Nodes.begin(), Nodes.end());
+ DenseMap<BPFunctionNode::UtilityNodeT, unsigned> UtilityNodeDegree;
+ for (auto &N : Nodes)
----------------
How much overhead does `DenseMap` have in comparison with `std::vector`? I assume one could first find the maximum utility index and then use a vector instead of a map. Of course, that should only be done if there is visible speedup.
================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:212
+ // Renumber utility nodes so they can be used to index into Signatures
+ DenseMap<BPFunctionNode::UtilityNodeT, unsigned> UtilityNodeIndex;
+ for (auto &N : Nodes)
----------------
maybe add `UtilityNodeIndex.reserve(NumUtilities)`
(and use the variable for signature initialization too)
================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:257-264
+ float cost = logCost(L, R);
+ float CostLR = 0, CostRL = 0;
+ if (L > 0)
+ CostLR = cost - logCost(L - 1, R + 1);
+ if (R > 0)
+ CostRL = cost - logCost(L + 1, R - 1);
+ Signature.CachedCostLR = CostLR;
----------------
================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:362
+ N.UtilityNodes.begin(), N.UtilityNodes.end(), 0.f,
+ [&](float Gain, auto &UN) { return Signatures[UN].CachedCostLR; });
+ } else {
----------------
should it be `Gain + Signatures[UN].CachedCostLR` instead?
Assuming I'm correct and this is a bug, can we add a test to catch the problem?
================
Comment at: llvm/lib/Support/BalancedPartitioning.cpp:366
+ N.UtilityNodes.begin(), N.UtilityNodes.end(), 0.f,
+ [&](float Gain, auto &UN) { return Signatures[UN].CachedCostRL; });
+ }
----------------
same here
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D147812/new/
https://reviews.llvm.org/D147812
More information about the llvm-commits
mailing list