[CodeLayout] Fix X1_Y_X2 and Y_X2_X1 testing for jumps from Y

[MaskRay]

The CHECK2 test in code_placement_ext_tsp_large.ll now has the same result as
the CHECK test: when chain(0,2,3,4,1) is merged with chain(8), the result is now
chain(0,2,3,4,8,1).

Ideally we should have test coverage for -ext-tsp-chain-split-threshold=1, but
it seems challenging to craft one. Perhaps the default value of
-ext-tsp-chain-split-threshold can be decreased as the
-ext-tsp-enable-chain-split-along-jumps heuristic is now more powerful.

[llvmbot]

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-llvm-transforms

Changes

The CHECK2 test in code_placement_ext_tsp_large.ll now has the same result as
the CHECK test. Ideally we should have test coverage for
-ext-tsp-chain-split-threshold=1, but it seems challenging to craft one.
Perhaps the default value of -ext-tsp-chain-split-threshold can be decreased
as the -ext-tsp-enable-chain-split-along-jumps heuristic is now more powerful.

---

Full diff: https://github.com/llvm/llvm-project/pull/66592.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Utils/CodeLayout.cpp (+1-1)
• (modified) llvm/test/CodeGen/X86/code_placement_ext_tsp_large.ll (+3-4)

diff --git a/llvm/lib/Transforms/Utils/CodeLayout.cpp b/llvm/lib/Transforms/Utils/CodeLayout.cpp
index c8c0823daaa6bd9..f020637ee186d38 100644
--- a/llvm/lib/Transforms/Utils/CodeLayout.cpp
+++ b/llvm/lib/Transforms/Utils/CodeLayout.cpp
@@ -850,7 +850,7 @@ class ExtTSPImpl {
 
       // Attach (a part of) ChainPred after the last node of ChainSucc.
       for (JumpT *Jump : ChainSucc->Nodes.back()->OutJumps) {
-        const NodeT *DstBlock = Jump->Source;
+        const NodeT *DstBlock = Jump->Target;
         if (DstBlock->CurChain != ChainPred)
           continue;
         size_t Offset = DstBlock->CurIndex;
diff --git a/llvm/test/CodeGen/X86/code_placement_ext_tsp_large.ll b/llvm/test/CodeGen/X86/code_placement_ext_tsp_large.ll
index 314df786b3e80b9..cee8489e9aaea0c 100644
--- a/llvm/test/CodeGen/X86/code_placement_ext_tsp_large.ll
+++ b/llvm/test/CodeGen/X86/code_placement_ext_tsp_large.ll
@@ -81,19 +81,18 @@ define void @func_large() !prof !0 {
 ; CHECK: b7
 ; CHECK: b9
 ;
-; An expected output with chain-split-threshold=1 (disabling splitting) -- the
-; increase of the layout score is smaller, ~7%:
+; An expected output with chain-split-threshold=1 (disabling split point enumeration)
 ;
 ; CHECK2-LABEL: Applying ext-tsp layout
 ; CHECK2:   original  layout score: 9171074274.27
-; CHECK2:   optimized layout score: 9810644873.57
+; CHECK2:   optimized layout score: 10844307310.87
 ; CHECK2: b0
 ; CHECK2: b2
 ; CHECK2: b3
 ; CHECK2: b4
 ; CHECK2: b5
-; CHECK2: b1
 ; CHECK2: b8
+; CHECK2: b1
 ; CHECK2: b6
 ; CHECK2: b7
 ; CHECK2: b9

[spupyrev]

wow that's indeed a bug, thanks for catching it! I'm running some internal tests
to see if there is a measurable improvement.
I wouldn't touch split-threshold, as it doesn't impact build time too much and
there are fairly straightforward ways of speeding up the implementation w/o
sacrificing the quality.

[MaskRay]

wow that's indeed a bug, thanks for catching it! I'm running some internal tests to see if there is a measurable improvement.

Thanks!

I wouldn't touch split-threshold, as it doesn't impact build time too much and there are fairly straightforward ways of speeding up the implementation w/o sacrificing the quality.

I am a bit concerned of the time complexity due to the large ChainSplitThreshold (128). If ChainSplitThreshold doesn't affect compile time that much, keeping the current value (128) should be fine. I just read through the code and haven't tested it on any real-world applications.