Skip to content

Commit f256b43

Browse files
YuriPlyakhinigcbot
YuriPlyakhin
authored and
igcbot
committed
Refactor HoistConvOpToDom and PromoteToPredicatedMemoryAccess
1. Update the HoistConvOpToDom pass to handle more corner cases. 2. Refactor the PromoteToPredicatedMemoryAccess pass to improve its structure and readability. 3. Add new tests to improve code coverage.
1 parent 6f3f54c commit f256b43

File tree

9 files changed

+317
-17
lines changed

9 files changed

+317
-17
lines changed

‎IGC/Compiler/Optimizer/HoistConvOpToDom.cpp‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -80,15 +80,23 @@ bool HoistConvOpToDom::runOnFunction(Function &F) {
8080
}
8181
if (!AllSame)
8282
continue;
83-
if (ToUpdate.empty())
84-
continue;
8583

86-
auto *CommonDominator = findNearestCommonDominator(PHI);
87-
if (!CommonDominator) {
88-
IGC_ASSERT_MESSAGE(0, "Common dominator not found");
84+
// Corner case: PHI with incoming values equal to the same first cast.
85+
// examples:
86+
// %phi = phi i32 [%conv1, %then]
87+
// %phi = phi i32 [%conv1, %then], [%conv1, %else]
88+
// etc...
89+
// just replace %phi with %conv1 and continue
90+
if (ToUpdate.empty()) {
91+
PHI.replaceAllUsesWith(FirstCast);
92+
PHI.eraseFromParent();
93+
Changed = true;
8994
continue;
9095
}
91-
FirstCast->moveBefore(CommonDominator->getTerminator());
96+
97+
auto *CommonDominator = findNearestCommonDominator(PHI);
98+
if(FirstCast->getParent() != CommonDominator)
99+
FirstCast->moveBefore(CommonDominator->getTerminator());
92100

93101
for (auto *I : ToUpdate) {
94102
LLVM_DEBUG(dbgs() << "HoistConvOpToDom: Replacing " << *I << " with " << *FirstCast << "\n");

‎IGC/Compiler/Optimizer/PromoteToPredicatedMemoryAccess.cpp‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ PromoteToPredicatedMemoryAccess::PromoteToPredicatedMemoryAccess() : FunctionPas
4848
}
4949

5050
bool PromoteToPredicatedMemoryAccess::runOnFunction(Function &F) {
51-
CodeGenContext *pCtx = getAnalysis<CodeGenContextWrapper>().getCodeGenContext();
51+
pCtx = getAnalysis<CodeGenContextWrapper>().getCodeGenContext();
5252

5353
bool isStatlessToBindlessEnabled =
5454
(pCtx->type == ShaderType::OPENCL_SHADER &&
@@ -83,8 +83,6 @@ bool PromoteToPredicatedMemoryAccess::runOnFunction(Function &F) {
8383
for (auto &[BI, Inverse] : WorkList) {
8484
auto *TrueBB = BI->getSuccessor(0);
8585
auto *FalseBB = BI->getSuccessor(1);
86-
87-
auto *SplitBB = BI->getParent();
8886
auto *TargetBB = Inverse ? FalseBB : TrueBB;
8987
auto *JoinBB = Inverse ? TrueBB : FalseBB;
9088

@@ -96,13 +94,13 @@ bool PromoteToPredicatedMemoryAccess::runOnFunction(Function &F) {
9694

9795
// Remove the phi nodes in the target block
9896
for (auto &Phi : make_early_inc_range(JoinBB->phis()))
99-
fixPhiNode(Phi, *TargetBB, *SplitBB);
97+
fixPhiNode(Phi, *TargetBB);
10098
}
10199

102100
return !WorkList.empty();
103101
}
104102

105-
void PromoteToPredicatedMemoryAccess::fixPhiNode(PHINode &Phi, BasicBlock &Predecessor, BasicBlock &CondBlock) {
103+
void PromoteToPredicatedMemoryAccess::fixPhiNode(PHINode &Phi, BasicBlock &Predecessor) {
106104
auto *IncomingV = Phi.getIncomingValueForBlock(&Predecessor);
107105
Phi.replaceAllUsesWith(IncomingV);
108106
Phi.eraseFromParent();
@@ -116,6 +114,8 @@ bool IsTypeLegal(Type *Ty) {
116114
unsigned Width = Ty->getScalarSizeInBits();
117115
return TypeLegalizer::isLegalInteger(Width) || Width == 1;
118116
}
117+
118+
bool IsLoadOkToConv(LoadInst *LI) { return LI->isSimple() && IsTypeLegal(LI->getType()); }
119119
} // namespace
120120

121121
bool PromoteToPredicatedMemoryAccess::trySingleBlockIfConv(Value &Cond, BasicBlock &BranchBB, BasicBlock &ConvBB,
@@ -165,7 +165,7 @@ bool PromoteToPredicatedMemoryAccess::trySingleBlockIfConv(Value &Cond, BasicBlo
165165
return false;
166166

167167
if (auto *LI = dyn_cast<LoadInst>(Inst)) {
168-
if (!LI->isSimple() || !IsTypeLegal(Inst->getType()))
168+
if (!IsLoadOkToConv(LI))
169169
return false;
170170

171171
SimpleInsts[Inst] = Phi.getIncomingValueForBlock(&BranchBB);
@@ -190,7 +190,7 @@ bool PromoteToPredicatedMemoryAccess::trySingleBlockIfConv(Value &Cond, BasicBlo
190190
return false;
191191

192192
auto *Load = dyn_cast<LoadInst>(EE->getVectorOperand());
193-
if (!Load || !Load->isSimple() || !IsTypeLegal(Load->getType()))
193+
if (!Load || !IsLoadOkToConv(Load))
194194
return false;
195195
if (Load->getParent() != &ConvBB)
196196
return false;
@@ -203,9 +203,11 @@ bool PromoteToPredicatedMemoryAccess::trySingleBlockIfConv(Value &Cond, BasicBlo
203203
}
204204
unsigned Idx = cast<ConstantInt>(EE->getIndexOperand())->getZExtValue();
205205
if (Idx >= MergeVector.size()) {
206-
LLVM_DEBUG(dbgs() << "Skip block, index " << Idx << " is more than vector size " << MergeVector.size() << "\n"
206+
std::string msg =
207+
"Index " + std::to_string(Idx) + " is >= vector size " + std::to_string(MergeVector.size());
208+
pCtx->EmitWarning(msg.c_str(), EE);
209+
LLVM_DEBUG(dbgs() << "Skip block. " << msg << "\n"
207210
<< "For ConvBB: " << ConvBB << "\n");
208-
IGC_ASSERT_MESSAGE(0, "Index is more than vector size");
209211
return false;
210212
}
211213
MergeVector[Idx] = Phi.getIncomingValueForBlock(&BranchBB);

‎IGC/Compiler/Optimizer/PromoteToPredicatedMemoryAccess.hpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,9 @@ class PromoteToPredicatedMemoryAccess : public llvm::FunctionPass {
6060
private:
6161
bool trySingleBlockIfConv(llvm::Value &Cond, llvm::BasicBlock &BranchBB, llvm::BasicBlock &ConvBB,
6262
llvm::BasicBlock &SuccBB, bool Inverse = false);
63-
6463
void convertMemoryAccesses(llvm::Instruction *Mem, llvm::Value *MergeV, llvm::Value *Cond, bool Inverse);
64+
void fixPhiNode(llvm::PHINode &Phi, llvm::BasicBlock &Predecessor);
6565

66-
void fixPhiNode(llvm::PHINode &Phi, llvm::BasicBlock &Predecessor, llvm::BasicBlock &CondBlock);
66+
CodeGenContext *pCtx = nullptr;
6767
};
6868
} // namespace IGC

‎IGC/Compiler/tests/HoistConvOpToDom/basic.ll‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,27 @@ merge:
6767
ret i32 %phi
6868
}
6969

70+
; CHECK-LABEL: @test_no_hoist_not_inst
71+
; CHECK: %conv1 = fptosi float %a to i32
72+
; CHECK: %phi = phi i32
73+
; CHECK: ret i32 %phi
74+
75+
define i32 @test_no_hoist_not_inst(float %a, i1 %cond) {
76+
entry:
77+
br i1 %cond, label %then, label %else
78+
79+
then:
80+
%conv1 = fptosi float %a to i32
81+
br label %merge
82+
83+
else:
84+
br label %merge
85+
86+
merge:
87+
%phi = phi i32 [%conv1, %then], [0, %else]
88+
ret i32 %phi
89+
}
90+
7091
; CHECK-LABEL: @test_no_hoist_different_src
7192
; CHECK: %conv1 = fptosi float %a to i32
7293
; CHECK: %conv2 = fptosi float %b to i32
@@ -126,3 +147,32 @@ merge:
126147
%phi = phi i32 [%conv1, %then2], [%conv2, %else2], [%conv3, %else]
127148
ret i32 %phi
128149
}
150+
151+
; CHECK-LABEL: @test_no_hoist_remove_phi
152+
; CHECK: entry:
153+
; CHECK: %conv1 = fptosi float %a to i32
154+
; CHECK: %use1 = add i32 %conv1, 1
155+
; CHECK: br i1 %cond, label %then, label %else
156+
; CHECK: then:
157+
; CHECK: br label %merge
158+
; CHECK: else:
159+
; CHECK: %use2 = add i32 %conv1, 1
160+
; CHECK: br label %merge
161+
; CHECK: merge:
162+
; CHECK: ret i32 %conv1
163+
164+
define i32 @test_no_hoist_remove_phi(float %a, i1 %cond) {
165+
entry:
166+
%conv1 = fptosi float %a to i32
167+
%use1 = add i32 %conv1, 1
168+
br i1 %cond, label %then, label %else
169+
then:
170+
br label %merge
171+
else:
172+
%conv2 = fptosi float %a to i32
173+
%use2 = add i32 %conv2, 1
174+
br label %merge
175+
merge:
176+
%phi = phi i32 [%conv1, %then], [%conv2, %else]
177+
ret i32 %phi
178+
}

‎IGC/Compiler/tests/HoistConvOpToDom/duplicate.ll‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,3 +49,43 @@ target:
4949
%result = phi i64 [ %different, %branch1 ], [ %duplicate, %branch2.1 ], [ %duplicate, %branch2.2 ]
5050
ret i64 %result
5151
}
52+
53+
; CHECK-LABEL: @test
54+
; CHECK: entry:
55+
; CHECK: %0 = bitcast float %arg0 to i32
56+
; CHECK: %duplicate = zext i32 %0 to i64
57+
; CHECK: br i1 %cond1, label %branch1, label %branch2
58+
; CHECK: branch1:
59+
; CHECK: br i1 %cond2, label %branch1.1, label %branch1.2
60+
; CHECK: branch1.1:
61+
; CHECK: br label %target
62+
; CHECK: branch1.2:
63+
; CHECK: br label %target
64+
; CHECK: branch2:
65+
; CHECK: br label %target
66+
; CHECK: target:
67+
; CHECK: ret i64 %duplicate
68+
69+
define i64 @test1(float %arg0, i1 %cond1, i1 %cond2) {
70+
entry:
71+
%0 = bitcast float %arg0 to i32
72+
br i1 %cond1, label %branch1, label %branch2
73+
74+
branch1:
75+
%duplicate = zext i32 %0 to i64
76+
br i1 %cond2, label %branch1.1, label %branch1.2
77+
78+
branch1.1:
79+
br label %target
80+
81+
branch1.2:
82+
br label %target
83+
84+
branch2:
85+
%different = zext i32 %0 to i64
86+
br label %target
87+
88+
target:
89+
%result = phi i64 [ %duplicate, %branch1.1 ], [ %duplicate, %branch1.2 ], [ %different, %branch2 ]
90+
ret i64 %result
91+
}

‎IGC/Compiler/tests/HoistConvOpToDom/first-cast-only.ll‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
;=========================== begin_copyright_notice ============================
2+
;
3+
; Copyright (C) 2025 Intel Corporation
4+
;
5+
; SPDX-License-Identifier: MIT
6+
;
7+
;============================ end_copyright_notice =============================
8+
9+
; RUN: igc_opt --igc-hoist-conv-op-to-dom --platformbmg -S < %s 2>&1 | FileCheck %s --implicit-check-not phi
10+
11+
; This test checks that the HoistConvOpToDom pass also removes redundant PHIs in corner case:
12+
; when the same conversion value is used in all incoming branches.
13+
14+
define i32 @test_1(float %a, i1 %cond) {
15+
; CHECK-LABEL: @test_1(
16+
; CHECK: [[CONV1:%.*]] = fptosi float
17+
; CHECK: merge:
18+
; CHECK-NEXT: ret i32 [[CONV1]]
19+
;
20+
entry:
21+
br label %then
22+
then:
23+
%conv1 = fptosi float %a to i32
24+
%use1 = add i32 %conv1, 1
25+
br label %merge
26+
merge:
27+
%phi = phi i32 [%conv1, %then]
28+
ret i32 %phi
29+
}
30+
31+
define i32 @test_2(float %a, i1 %cond) {
32+
; CHECK-LABEL: @test_2(
33+
; CHECK: [[CONV1:%.*]] = fptosi float
34+
; CHECK: merge:
35+
; CHECK-NEXT: ret i32 [[CONV1]]
36+
;
37+
entry:
38+
%conv1 = fptosi float %a to i32
39+
br i1 %cond, label %then, label %else
40+
then:
41+
%use1 = add i32 %conv1, 1
42+
br label %merge
43+
else:
44+
br label %merge
45+
merge:
46+
%phi = phi i32 [%conv1, %then], [%conv1, %else]
47+
ret i32 %phi
48+
}
49+
50+
define i32 @test_multiple(float %a, i1 %cond1, i1 %cond2) {
51+
; CHECK-LABEL: @test_multiple(
52+
; CHECK: [[CONV1:%.*]] = fptosi float
53+
; CHECK: merge:
54+
; CHECK-NEXT: ret i32 [[CONV1]]
55+
;
56+
entry:
57+
%conv1 = fptosi float %a to i32
58+
br i1 %cond1, label %then, label %else
59+
then:
60+
br i1 %cond2, label %then2, label %else2
61+
then2:
62+
%use1 = add i32 %conv1, 1
63+
br label %merge
64+
else2:
65+
br label %merge
66+
else:
67+
br label %merge
68+
merge:
69+
%phi = phi i32 [%conv1, %then2], [%conv1, %else2], [%conv1, %else]
70+
ret i32 %phi
71+
}

‎IGC/Compiler/tests/PromoteToPredicatedMemoryAccess/load.ll‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,3 +197,20 @@ exit:
197197
%res = phi i32 [ %load, %st ], [ %data, %entry ]
198198
ret i32 %res
199199
}
200+
201+
; Test pass does nothing, if nothing to convert
202+
; CHECK-LABEL: @test10(
203+
define i32 @test10(i1 %pred) {
204+
entry:
205+
; CHECK: br i1 %pred, label %st, label %exit
206+
br i1 %pred, label %st, label %exit
207+
208+
st:
209+
; CHECK: %a = add i32 3, 8
210+
%a = add i32 3, 8
211+
br label %exit
212+
213+
exit:
214+
; CHECK: ret i32 42
215+
ret i32 42
216+
}

0 commit comments

Comments
 (0)