[PATCH] D113424: ext-tsp basic block layout

[Sergey Pupyrev via Phabricator via llvm-commits]

spupyrev added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3472
+    int NumInsts = std::distance(NonDbgInsts.begin(), NonDbgInsts.end());
+    BlockSizes[BlockIndex[&MBB]] = 4 * NumInsts;
+    // Getting jump frequencies.
----------------
davidxl wrote:
> Why not computing the actual byte size of the block?
That would be great! Do you mind pointing to the right way of implementing this? I was not able to find anything meaningless in MachineBasicBlock


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

The goal is here is to make applyExtTspLayout work with any type of nodes, not just MachineBasicBlock (potentially we want to apply it for function sorting). I tried creating a generic implementation with templates (and thus "hiding" NewOrder) but it turned out to be much messier than what it is now.


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3529
+  for (const MachineBasicBlock *MBB : NewBlockOrder) {
+    NewIndex[MBB] = NewIndex.size();
+  }
----------------
davidxl wrote:
> Is NewIndex the same as BlockIndex computed in applyExtTsp? Why recomputing?
No, this is a new index after reordering. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113424