[PATCH] D113424: ext-tsp basic block layout

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3457
+
+  DenseMap<uint64_t, uint64_t> BlockSizes;
+  BlockSizes.reserve(F->size());
----------------
It is better to use vector type as the index space is dense?


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3459
+  BlockSizes.reserve(F->size());
+  DenseMap<uint64_t, uint64_t> BlockCounts;
+  BlockCounts.reserve(F->size());
----------------
vector type?


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3472
+    int NumInsts = std::distance(NonDbgInsts.begin(), NonDbgInsts.end());
+    BlockSizes[BlockIndex[&MBB]] = 4 * NumInsts;
+    // Getting jump frequencies.
----------------
Why not computing the actual byte size of the block?


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3478
+      auto Edge = std::make_pair(BlockIndex[&MBB], BlockIndex[Succ]);
+      JumpCounts[Edge] += EdgeFreq.getFrequency();
+    }
----------------
Is multi-graph assumed, why?


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3492
+  auto NewOrder = applyExtTspLayout(BlockSizes, BlockCounts, JumpCounts);
+  std::vector<const MachineBasicBlock *> NewBlockOrder;
+  NewBlockOrder.reserve(F->size());
----------------
Can the following (computing NewBlockOrder) be folded into applyExtTspLayout?  The NewOrder is not used anywhere else, so no need to be exposed.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3529
+  for (const MachineBasicBlock *MBB : NewBlockOrder) {
+    NewIndex[MBB] = NewIndex.size();
+  }
----------------
Is NewIndex the same as BlockIndex computed in applyExtTsp? Why recomputing?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113424