[PATCH] D113424: ext-tsp basic block layout

[David Li via Phabricator via llvm-commits]

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