[PATCH] D113424: ext-tsp basic block layout
David Li via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 23 09:53:37 PST 2021
davidxl added inline comments.
================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:85
+// misses for a given ordering of basic blocks
+double extTSPScore(uint64_t SrcAddr, uint64_t SrcSize, uint64_t DstAddr,
+ uint64_t Count) {
----------------
The paper documents 6 types of branches and their weight K_{s,t}. Should these be parametized here?
================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:97
+ double Prob = 1.0 - static_cast<double>(Dist) / ForwardDistance;
+ return ForwardWeight * Prob * Count;
+ }
----------------
The tsp score does not seem to be contiguous here. When Dist == 0, it should have the same score as the fall through case, but this produces 0.1* Count.
================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:639
+ /// Compute Ext-TSP score for a given block order and a given list of jumps.
+ double score(const MergedChain &MergedBlocks,
+ const std::vector<Jump *> &Jumps) const {
----------------
It is confusing to have both this method and calcExtTSPScore methods.
================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:926
+ for (size_t Idx = 0; Idx < Order.size(); Idx++) {
+ BlockIndex[Order[Idx]] = Idx;
+ }
----------------
>From the initialization, Order[Idx] == Idx, so is this map needed?
================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:935
+ assert(BlockIndex.find(Succ) != BlockIndex.end() && "Block not found");
+ // Incresing the score if the two nodes are adjacent in the order.
+ if (BlockIndex[Pred] + 1 == BlockIndex[Succ])
----------------
typo 'Increasing'
================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:937
+ if (BlockIndex[Pred] + 1 == BlockIndex[Succ])
+ Score += It.second;
+ }
----------------
why is the count not used?
================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:939
+ }
+ return Score;
+}
----------------
This function computes the 'fallthrough' maximizing TSP score, not the extTSP in the original paper. Where is the h function and the K coeffcients?
================
Comment at: llvm/lib/Transforms/Utils/CodeLayout.cpp:947
+ std::vector<uint64_t> Order;
+ Order.reserve(NodeSizes.size());
+ for (size_t Idx = 0; Idx < NodeSizes.size(); Idx++) {
----------------
Can the reserve be combined with the vector declaration, or the intention is to avoid initialization?
================
Comment at: llvm/test/CodeGen/X86/code_placement_ext_tsp_large.ll:9
+; A larger (randomly generated) CFG instance where chain splitting helps to
+; compute a better basic block ordering
+;
----------------
spupyrev wrote:
> davidxl wrote:
> > Add a cfg with ascii art.
> The purpose of the test is to verify that chain splitting code is executed and that it is beneficial for the layout, that is, the optimized score is increased. I added the explanations.
>
> Since the graph is large and randomly generated, the ascii representation is not very readable. In fact, it is not providing any insights, so let's skip it?
ok. I was expecting to look into the test case and reason that improved score corresponds to improved layout. Looks like this is just a test to show increased score. Can this be done instead with any other tests (the score should be increased monotonically) ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D113424/new/
https://reviews.llvm.org/D113424
More information about the llvm-commits
mailing list