[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:573
+  /// Create a single CFG chain from the current block order.
+  void createCFGChain();
+
----------------
Perhaps make the name clearer: createCFGChainExtTSP()?


================
Comment at: llvm/lib/CodeGen/MachineBlockPlacement.cpp:3408
+    // Find a new placement and modify the layout of the blocks in the function.
+    applyExtTsp();
+
----------------
Why do the extTSP layout after the existing layout? It seems like a waste of compile time.

Or the intention is to keep the taildup functionality there? However the tail dup decisions depend on layout decisions, so it is probably better integrated with extTSP.  Also taildup's profile update may not be ideal affecting later layout score computation.


================
Comment at: llvm/test/CodeGen/X86/code_placement_ext_tsp.ll:1
+; RUN: llc -mcpu=corei7 -mtriple=x86_64-linux -enable-ext-tsp-block-placement=1 < %s | FileCheck %s
+
----------------
rahmanl wrote:
> The interesting code path (splitting the chain) is not exercised by these tests. Please add a test for that.
The extTSP algorithm should be able to handle common loop rotation case. Can you add a test case about it?


================
Comment at: llvm/test/CodeGen/X86/code_placement_ext_tsp.ll:13
+  %call = call zeroext i1 @a()
+  br i1 %call, label %b1, label %b2, !prof !1
+
----------------
We can reason that it requires double the weights to the merged block compared with the side branch of the triangular shaped CFG to make non-topological order to be profitable. In this case it is 100> 2* 40. Can you add a negative case that keeps the top order?


================
Comment at: llvm/test/CodeGen/X86/code_placement_ext_tsp.ll:28
+define void @func2() !prof !2 {
+; Test that the placement positions the hot chain is placed continuosly
+;
----------------
Fix typo


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113424