[StructurizeCFG] Hoist and simplify zero-cost incoming else phi values #139605
Conversation
Then and Else block order in SCC is arbitrary. But based on the order, after structurization there are cases where there might be extra VGPR copies due to interference during register coelescing. This patch introduces heuristics to order the then and else block based on the potential VGPR copies to maximize coelescing.
|
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-llvm-transforms Author: Vigneshwar Jayakumar (VigneshwarJ) ChangesThen and Else block order in SCC is arbitrary. But based on the order, after structurization there are cases where there might be extra VGPR copies due to interference during register coelescing. This patch introduces heuristics to order the then and else block based on the potential VGPR copies to maximize coelescing. Full diff: https://github.com/llvm/llvm-project/pull/139605.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp b/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp
index eb22b50532695..ec54f53d6165b 100644
--- a/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp
+++ b/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp
@@ -307,6 +307,8 @@ class StructurizeCFG {
RegionNode *PrevNode;
+ void reorderIfElseBlock(BasicBlock *BB, unsigned Idx);
+
void orderNodes();
void analyzeLoops(RegionNode *N);
@@ -409,6 +411,31 @@ class StructurizeCFGLegacyPass : public RegionPass {
} // end anonymous namespace
+/// Helper function for heuristics to order if else block
+/// Checks whether an instruction is potential vector copy instruction, if so,
+/// checks if the operands are from different BB. if so, returns True.
+// Then there's a possibility of coelescing without interference when ordered
+// first.
+static bool hasAffectingInstructions(Instruction *I, BasicBlock *BB) {
+
+ if (!I || I->getParent() != BB)
+ return true;
+
+ // If the instruction is not a poterntial copy instructoin, return true.
+ if (!isa<ExtractElementInst>(*I) && !isa<ExtractValueInst>(*I))
+ return false;
+
+ // Check if any operands are instructions defined in the same block.
+ for (unsigned i = 0, e = I->getNumOperands(); i < e; ++i) {
+ if (auto *OpI = dyn_cast<Instruction>(I->getOperand(i))) {
+ if (OpI->getParent() == BB)
+ return false;
+ }
+ }
+
+ return true;
+}
+
char StructurizeCFGLegacyPass::ID = 0;
INITIALIZE_PASS_BEGIN(StructurizeCFGLegacyPass, "structurizecfg",
@@ -419,6 +446,58 @@ INITIALIZE_PASS_DEPENDENCY(RegionInfoPass)
INITIALIZE_PASS_END(StructurizeCFGLegacyPass, "structurizecfg",
"Structurize the CFG", false, false)
+/// Then and Else block order in SCC is arbitrary. But based on the
+/// order, after structurization there are cases where there might be extra
+/// VGPR copies due to interference during register coelescing.
+/// eg:- incoming phi values from Else block contains only vgpr copies and
+/// incoming phis in Then block has are some modification for the vgprs.
+/// after structurization, there would be interference when coelesing when Then
+/// block is ordered first. But those copies can be coelesced when Else is
+/// ordered first.
+///
+/// This function checks the incoming phi values in the merge block and
+/// orders based on the following heuristics of Then and Else block. Checks
+/// whether an incoming phi can be potential copy instructions and if so
+/// checks whether copy within the block or not.
+/// Increases score if its a potential copy from outside the block.
+/// the higher scored block is ordered first.
+void StructurizeCFG::reorderIfElseBlock(BasicBlock *BB, unsigned Idx) {
+ BranchInst *Term = dyn_cast<BranchInst>(BB->getTerminator());
+
+ if (Term && Term->isConditional()) {
+ BasicBlock *ThenBB = Term->getSuccessor(0);
+ BasicBlock *ElseBB = Term->getSuccessor(1);
+ BasicBlock *ThenSucc = ThenBB->getSingleSuccessor();
+
+ if (BB == ThenBB->getSinglePredecessor() &&
+ (ThenBB->getSinglePredecessor() == ElseBB->getSinglePredecessor()) &&
+ (ThenSucc && ThenSucc == ElseBB->getSingleSuccessor())) {
+ unsigned ThenScore = 0, ElseScore = 0;
+
+ for (PHINode &Phi : ThenSucc->phis()) {
+ Value *ThenVal = Phi.getIncomingValueForBlock(ThenBB);
+ Value *ElseVal = Phi.getIncomingValueForBlock(ElseBB);
+
+ if (auto *Inst = dyn_cast<Instruction>(ThenVal))
+ ThenScore += hasAffectingInstructions(Inst, ThenBB);
+ if (auto *Inst = dyn_cast<Instruction>(ElseVal))
+ ElseScore += hasAffectingInstructions(Inst, ElseBB);
+ }
+
+ if (ThenScore != ElseScore) {
+ if (ThenScore < ElseScore)
+ std::swap(ThenBB, ElseBB);
+
+ // reorder the last two inserted elements in Order
+ if (Idx >= 2 && Order[Idx - 1]->getEntry() == ElseBB &&
+ Order[Idx - 2]->getEntry() == ThenBB) {
+ std::swap(Order[Idx - 1], Order[Idx - 2]);
+ }
+ }
+ }
+ }
+}
+
/// Build up the general order of nodes, by performing a topological sort of the
/// parent region's nodes, while ensuring that there is no outer cycle node
/// between any two inner cycle nodes.
@@ -452,6 +531,7 @@ void StructurizeCFG::orderNodes() {
// Add the SCC nodes to the Order array.
for (const auto &N : SCC) {
assert(I < E && "SCC size mismatch!");
+ reorderIfElseBlock(N.first->getEntry(), I);
Order[I++] = N.first;
}
}
diff --git a/llvm/test/Transforms/StructurizeCFG/order-if-else.ll b/llvm/test/Transforms/StructurizeCFG/order-if-else.ll
new file mode 100644
index 0000000000000..02641f405f3b4
--- /dev/null
+++ b/llvm/test/Transforms/StructurizeCFG/order-if-else.ll
@@ -0,0 +1,129 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -structurizecfg %s -o - | FileCheck %s
+; RUN: opt -S -passes=structurizecfg %s -o - | FileCheck %s
+
+define amdgpu_kernel void @test_extractelement_1(<4 x i32> %vec, i1 %cond, ptr %ptr) {
+; CHECK-LABEL: define amdgpu_kernel void @test_extractelement_1(
+; CHECK-SAME: <4 x i32> [[VEC:%.*]], i1 [[COND:%.*]], ptr [[PTR:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: [[COND_INV:%.*]] = xor i1 [[COND]], true
+; CHECK-NEXT: br i1 [[COND_INV]], label %[[ELSE:.*]], label %[[FLOW:.*]]
+; CHECK: [[FLOW]]:
+; CHECK-NEXT: [[TMP0:%.*]] = phi i32 [ [[A:%.*]], %[[ELSE]] ], [ poison, %[[ENTRY]] ]
+; CHECK-NEXT: [[TMP1:%.*]] = phi i1 [ false, %[[ELSE]] ], [ true, %[[ENTRY]] ]
+; CHECK-NEXT: br i1 [[TMP1]], label %[[THEN:.*]], label %[[MERGE:.*]]
+; CHECK: [[THEN]]:
+; CHECK-NEXT: [[X:%.*]] = extractelement <4 x i32> [[VEC]], i32 0
+; CHECK-NEXT: [[Z:%.*]] = add i32 [[X]], 1
+; CHECK-NEXT: br label %[[MERGE]]
+; CHECK: [[ELSE]]:
+; CHECK-NEXT: [[A]] = extractelement <4 x i32> [[VEC]], i32 1
+; CHECK-NEXT: br label %[[FLOW]]
+; CHECK: [[MERGE]]:
+; CHECK-NEXT: [[PHI:%.*]] = phi i32 [ [[TMP0]], %[[FLOW]] ], [ [[Z]], %[[THEN]] ]
+; CHECK-NEXT: store i32 [[PHI]], ptr [[PTR]], align 4
+; CHECK-NEXT: ret void
+;
+entry:
+ br i1 %cond, label %then, label %else
+
+then:
+ %x = extractelement <4 x i32> %vec, i32 0
+ %z = add i32 %x, 1
+ br label %merge
+
+else:
+ %a = extractelement <4 x i32> %vec, i32 1
+ br label %merge
+
+merge:
+ %phi = phi i32 [ %z, %then ], [ %a, %else ]
+ store i32 %phi, ptr %ptr
+ ret void
+}
+
+define amdgpu_kernel void @test_extractelement_2(<4 x i32> %vec, i1 %cond, ptr %ptr) {
+; CHECK-LABEL: define amdgpu_kernel void @test_extractelement_2(
+; CHECK-SAME: <4 x i32> [[VEC:%.*]], i1 [[COND:%.*]], ptr [[PTR:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: [[COND_INV:%.*]] = xor i1 [[COND]], true
+; CHECK-NEXT: br i1 [[COND_INV]], label %[[ELSE:.*]], label %[[FLOW:.*]]
+; CHECK: [[FLOW]]:
+; CHECK-NEXT: [[TMP0:%.*]] = phi i32 [ [[A:%.*]], %[[ELSE]] ], [ poison, %[[ENTRY]] ]
+; CHECK-NEXT: [[TMP1:%.*]] = phi i1 [ false, %[[ELSE]] ], [ true, %[[ENTRY]] ]
+; CHECK-NEXT: br i1 [[TMP1]], label %[[THEN:.*]], label %[[MERGE:.*]]
+; CHECK: [[THEN]]:
+; CHECK-NEXT: [[X:%.*]] = extractelement <4 x i32> [[VEC]], i32 1
+; CHECK-NEXT: [[Y:%.*]] = add i32 [[X]], 1
+; CHECK-NEXT: [[VEC1:%.*]] = insertelement <4 x i32> poison, i32 [[Y]], i32 0
+; CHECK-NEXT: [[Z:%.*]] = extractelement <4 x i32> [[VEC1]], i32 0
+; CHECK-NEXT: br label %[[MERGE]]
+; CHECK: [[ELSE]]:
+; CHECK-NEXT: [[A]] = extractelement <4 x i32> [[VEC]], i32 1
+; CHECK-NEXT: br label %[[FLOW]]
+; CHECK: [[MERGE]]:
+; CHECK-NEXT: [[PHI:%.*]] = phi i32 [ [[TMP0]], %[[FLOW]] ], [ [[Z]], %[[THEN]] ]
+; CHECK-NEXT: store i32 [[PHI]], ptr [[PTR]], align 4
+; CHECK-NEXT: ret void
+;
+entry:
+ br i1 %cond, label %then, label %else
+
+then:
+ %x = extractelement <4 x i32> %vec, i32 1
+ %y = add i32 %x, 1
+ %vec1 = insertelement <4 x i32> poison, i32 %y, i32 0
+ %z = extractelement <4 x i32> %vec1, i32 0
+ br label %merge
+
+else:
+ %a = extractelement <4 x i32> %vec, i32 1
+ br label %merge
+
+merge:
+ %phi = phi i32 [ %z, %then ], [ %a, %else ]
+ store i32 %phi, ptr %ptr
+ ret void
+}
+
+%pair = type { i32, i32 }
+define amdgpu_kernel void @test_extractvalue(ptr %ptr, i1 %cond) {
+; CHECK-LABEL: define amdgpu_kernel void @test_extractvalue(
+; CHECK-SAME: ptr [[PTR:%.*]], i1 [[COND:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: [[LOAD_THEN:%.*]] = load [[PAIR:%.*]], ptr [[PTR]], align 4
+; CHECK-NEXT: br i1 [[COND]], label %[[THEN:.*]], label %[[FLOW:.*]]
+; CHECK: [[THEN]]:
+; CHECK-NEXT: [[A_THEN:%.*]] = extractvalue [[PAIR]] [[LOAD_THEN]], 0
+; CHECK-NEXT: br label %[[FLOW]]
+; CHECK: [[FLOW]]:
+; CHECK-NEXT: [[TMP0:%.*]] = phi i32 [ [[A_THEN]], %[[THEN]] ], [ poison, %[[ENTRY]] ]
+; CHECK-NEXT: [[TMP1:%.*]] = phi i1 [ false, %[[THEN]] ], [ true, %[[ENTRY]] ]
+; CHECK-NEXT: br i1 [[TMP1]], label %[[ELSE:.*]], label %[[MERGE:.*]]
+; CHECK: [[ELSE]]:
+; CHECK-NEXT: [[A_ELSE:%.*]] = extractvalue [[PAIR]] [[LOAD_THEN]], 0
+; CHECK-NEXT: [[SUM_ELSE:%.*]] = add i32 [[A_ELSE]], 1
+; CHECK-NEXT: br label %[[MERGE]]
+; CHECK: [[MERGE]]:
+; CHECK-NEXT: [[PHI:%.*]] = phi i32 [ [[TMP0]], %[[FLOW]] ], [ [[SUM_ELSE]], %[[ELSE]] ]
+; CHECK-NEXT: store i32 [[PHI]], ptr [[PTR]], align 4
+; CHECK-NEXT: ret void
+;
+entry:
+ %load_then = load %pair, ptr %ptr
+ br i1 %cond, label %then, label %else
+
+then:
+ %a_then = extractvalue %pair %load_then, 0
+ br label %merge
+
+else:
+ %a_else = extractvalue %pair %load_then, 0
+ %sum_else = add i32 %a_else, 1
+ br label %merge
+
+merge:
+ %phi = phi i32 [ %a_then, %then ], [ %sum_else, %else ]
+ store i32 %phi, ptr %ptr
+ ret void
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| if (!isa<ExtractElementInst>(*I) && !isa<ExtractValueInst>(*I)) | ||
| return false; |
There was a problem hiding this comment.
Don't understand looking for special case instruction types
There was a problem hiding this comment.
The best way these heuristics will work is if we know which are going to be copy instructions after lowering. Here I just look at these two because mostly these are getting lowered to copy instruction. Another way of calculating heuristics for ordering was to see whether the incoming phi value instruction defined by the operands of the same block or different block. but this is too broad and can have unintentional reorders.
| /// Then and Else block order in SCC is arbitrary. But based on the | ||
| /// order, after structurization there are cases where there might be extra |
There was a problem hiding this comment.
Can we just sort blocks in topological order or something? I'm not sure I understand this heuristic
There was a problem hiding this comment.
The blocks are already sorted in the topological order. But in case of if-then-else, topologically then,else or else,then is same. But based on the order, there are extra copies that leads to spill in large cases.
This is specifically seen in case where one block just copies values and other block modifies values.
godbolt link
if:
%vec = <4 x i32> ...
br i1 %c %then %else
then:
%x = extractelement <4 x i32> %vec, i32 0
%z = add i32 %x, 1
br label %merge
else:
%a = extractelement <4 x i32> %vec, i32 0
br label %merge
merge:
%phi = phi i32 [ %z, %then ], [ %a, %else ]
store i32 %phi, ptr %ptr
ret void
After structurization and machine code , if 'then' block is ordered first
then at register coalescer
if:
[v0 - v4] = ...
cond_branch_scc then
tmp:
v5 = 0
branch Flow
then:
v5 = copy v0 ; eliminated in reg coalescer
v5 = v5 + 1 ; v5 = v0 + 1
flow:
v6 = v5 ; eliminated
cond_branch final
else:
v6 = copy v0 ; v5 = copy v0
; this copy v5 = copy v0 can't be coalesced further then->flow->else live range
final:
store v6 ; store v5but at the same time, if 'else' block is ordered first,
if:
[v0 - v4] = ...
cond_branch_scc else
tmp:
v5 = 0
branch Flow
else:
v5 = copy v0 ; eliminated
flow:
v6 = copy v5 ; eliminated
cond_branch final
then:
v6 = copy v0 ; eliminated
v6 = v6 + 1 ; v6 = v0 + 1 ; can be coalesced to v0 = v0 + 1
final:
store v6 ; coalesced to store v0Theres an extra copy just due to ordering in the first case. Though then and else block won't be live at same time due to structurization, we see this.
This heuristics just sees instructions that can be lowered to copy instructions
There was a problem hiding this comment.
Anything can be lowered to copied, initially most cross block values emit copies. Does this really mean "0" cost instructions?
There was a problem hiding this comment.
Yes, changed the code to check for zero cost instructions
|
It feels like we hit the limitation in the register coalescer that it cannot do coalescing in the optimal way, and it is hard to solve the problem there. But ordering the blocks does not feel like the optimal way to solve the problem. Think what if you have two simultaneous values, one is modified in If you look at the IR before structurization, there is only one value alive at any program point. But after structurization, we get something like: This is quite like the problem I solved in #101301, but more challenging. Can we try to simplify the phi by moving the extractelemnt into dominator? Like: By doing this, we still have only one copy of the value being alive on the CFG post-structurization. If we allocate VGPR on thread-CFG, this would be trivially solved, but it is a large project. |
|
@ruiling Thanks for the suggestion. if:
%load_then = load %pair, ptr %ptr
br i1 %cond, label %then, label %else
then:
%a_then = extractvalue %pair %load_then, 0
br label %merge
else:
%a_else = extractvalue %pair %load_then, 0
%sum_else = add i32 %a_else, 1
br label %merge
merge:
%phi = phi i32 [ %a_then, %then ], [ %sum_else, %else ]
store i32 %phi, ptr %ptr
ret voidstructurizeCFG in trunk - https://godbolt.org/z/Gc6Wbocef ISA with structurizeCFG with hoisting zero-cost instruction change - https://godbolt.org/z/cEzs31dYq. There is an extra copy even with hoisting change because, in the default SCC order, 'else' block comes first arbitrarily. Though the 'then' block doesn't contain any code now, 'copy' can't be coalesced due to interference (else->flow->then path) like I mentioned in Matt's comment. In the above example, using a pass like gvnhoist/simplifycfg would have eliminated the 'then' block after hoisting and simplifying. But if that block contains any other instructions, then the block will not be eliminated, and the problem will still exist. But yes, the reorder heuristics is not perfect and will not work for the case you mentioned. But I felt that's still a better trade-off than the current implementation. |
You missed something important. Hoisting is only part of the suggestion. The phi network also needs some change. |
VigneshwarJ
changed the title
VigneshwarJ
changed the title
|
@ruiling , Sorry that was my bad, I completely missed that point. Fixed it now. |
| @@ -535,7 +613,7 @@ void StructurizeCFG::gatherPredicates(RegionNode *N) { | |||
| BasicBlock *Other = Term->getSuccessor(!i); | |||
| if (Visited.count(Other) && !Loops.count(Other) && | |||
| !Pred.count(Other) && !Pred.count(P)) { | |||
|
|
|||
| hoistZeroCostElseBlockPhiValues(Succ, Other); | |||
There was a problem hiding this comment.
Added hoisting logic here, because the else block for structurized CFG is getting defined here. And there's no point in hoisting then block zero-cost instruction phis.
| @@ -413,6 +455,41 @@ INITIALIZE_PASS_DEPENDENCY(RegionInfoPass) | |||
| INITIALIZE_PASS_END(StructurizeCFGLegacyPass, "structurizecfg", | |||
| "Structurize the CFG", false, false) | |||
|
|
|||
| /// Because the SCC order of Then and Else blocks is arbitrary, structurization | |||
There was a problem hiding this comment.
Talking about the arbitrary order of then/else block is a little bit confusing here. Because we treated the first visited successor as Then block, and the other one as Else block throughout the code implementation. Maybe just remove it to avoid such confusion.
| @@ -0,0 +1,163 @@ | |||
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5 | |||
| ; RUN: opt -S -structurizecfg %s -o - | FileCheck %s | |||
There was a problem hiding this comment.
I think -structurizecfg is the old way to run pass with opt? We only need the second run.
| /// if its hoisted to predecessor block. So, this returns true. | ||
| static bool isHoistableInstruction(Instruction *I, BasicBlock *BB, | ||
| TargetTransformInfo *TTI) { | ||
|
|
| return false; | ||
|
|
||
| // If the instruction is not a zero cost instruction, return false. | ||
| auto Cost = TTI->getInstructionCost(I, TargetTransformInfo::TCK_CodeSize); |
| return false; | ||
|
|
||
| // Check if any operands are instructions defined in the same block. | ||
| for (unsigned i = 0, e = I->getNumOperands(); i < e; ++i) { |
| break; | ||
| } | ||
| if (PoisonValBBIdx == -1 || | ||
| !DT->dominates(HoistedValues[V], |
There was a problem hiding this comment.
Double map lookup, save the iterator in a find above instead of using count
| @@ -0,0 +1,180 @@ | |||
| ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5 | |||
| ; RUN: llc -mtriple=amdgcn -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck -check-prefix=GFX900 %s | |||
There was a problem hiding this comment.
| ; RUN: llc -mtriple=amdgcn -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck -check-prefix=GFX900 %s | |
| ; RUN: llc -mtriple=amdgcn -mcpu=gfx900 < %s | FileCheck -check-prefix=GFX900 %s |
|
|
||
|
|
| @@ -0,0 +1,162 @@ | |||
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5 | |||
| ; RUN: opt -S -passes=structurizecfg %s -o - | FileCheck %s | |||
There was a problem hiding this comment.
| ; RUN: opt -S -passes=structurizecfg %s -o - | FileCheck %s | |
| ; RUN: opt -S -passes=structurizecfg < %s | FileCheck %s |
The order of if and else blocks can introduce unnecessary VGPR copies.
Consider the case of an if-else block where the incoming phi from the 'Else block' only contains zero-cost instructions, and the 'Then' block modifies some value. There would be no interference when coalescing because only one value is live at any point before structurization. However, in the structurized CFG, the Then value is live at 'Else' block due to the path if→flow→else, leading to additional VGPR copies.
This patch addresses the issue by: