[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:3571
+  MachineBasicBlock *HeadBB = &F->front();
+  BlockChain *FunctionChain =
+      new (ChainAllocator.Allocate()) BlockChain(BlockToChain, HeadBB);
----------------
hoy wrote:
> The object is created but not used anywhere. Is that expected?
What object do you refer to?


================
Comment at: llvm/test/CodeGen/X86/code_placement_ext_tsp.ll:303
+;
+; With chain splitting disabled:
+; CHECK2-LABEL: func4:
----------------
davidxl wrote:
> This might be a bug in existing layout. By design, with profile data,  in order to take b2 as the layout successor, the incoming edge weight needs > 2*10 = 20.
This tests uses two different modes of ext-tsp, with and without "chain splitting" -- something that Rahman suggested. The "old" layout in MachineBlockPlacement isn't applied here.


================
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
+;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113424