Skip to content

Commit 19a6bb5

Browse files
author
Joe Shajrawi
committed
[LICM] Code Hygiene - rip out sinkCondFail
Removing this optimization from SIL: It is not worth the extra code complexity and compilation time. More in-depth explanation for the reasoning behind my decision: 1) What is being done there is obviously not LICM (more below) - even if it is useful it should be its own separate optimization 2) The regression that caused us to add this code is no longer there in most cases - 10% in only one specific corner-case 3) Even if the regression was still there, this is an extremely specific code pattern that we are pattern-matching against. Said pattern would be hard to find in any real code. There is a small code snippet in rdar://17451529 that caused us to add this optimization. Looking at it now we see that the only difference is in loop1 example - The only difference in SIL level is in loop 1: %295 = tuple_extract %294 : $(Builtin.Int64, Builtin.Int1), 0 %296 = tuple_extract %294 : $(Builtin.Int64, Builtin.Int1), 1 cond_fail %296 : $Builtin.Int1 %298 = struct $Int (%295 : $Builtin.Int64) store %298 to %6 : $*Int %300 = builtin "cmp_eq_Int64"(%292 : $Builtin.Int64, %16 : $Builtin.Int64) : $Builtin.Int1 cond_br %300, bb1, bb12 The cond_fail instruction in said loop is moved below the store instruction / above the builtin. Looking at the resulting IR. And how LLVM optimizes it. It is almost the same. If we look at the assembly code being executed then, before removing this optimization, we have: LBB0_11: testq %rcx, %rcx je LBB0_2 decq %rcx incq %rax movq %rax, _$S4main4sum1Sivp(%rip) jno LBB0_11 After removing it we have: LBB0_11: incq %rax testq %rcx, %rcx je LBB0_2 decq %rcx movq %rax, %rdx incq %rdx jno LBB0_11 There is no extra load/movq which was mentioned the radar.
1 parent 1ba5415 commit 19a6bb5

File tree

2 files changed

+0
-139
lines changed

2 files changed

+0
-139
lines changed

lib/SILOptimizer/LoopTransforms/LICM.cpp

Lines changed: 0 additions & 76 deletions
Original file line numberDiff line numberDiff line change
@@ -125,81 +125,6 @@ static bool hasLoopInvariantOperands(SILInstruction *I, SILLoop *L) {
125125
});
126126
}
127127

128-
/// Check if an address does not depend on other values in a basic block.
129-
static SILInstruction *addressIndependent(SILValue Addr) {
130-
Addr = stripCasts(Addr);
131-
if (auto *SGAI = dyn_cast<GlobalAddrInst>(Addr))
132-
return SGAI;
133-
if (auto *SEAI = dyn_cast<StructElementAddrInst>(Addr))
134-
return addressIndependent(SEAI->getOperand());
135-
return nullptr;
136-
}
137-
138-
/// Check if two addresses can potentially access the same memory.
139-
/// For now, return true when both can be traced to the same global variable.
140-
static bool addressCanPairUp(SILValue Addr1, SILValue Addr2) {
141-
SILInstruction *Origin1 = addressIndependent(Addr1);
142-
return Origin1 && Origin1 == addressIndependent(Addr2);
143-
}
144-
145-
/// Move cond_fail down if it can potentially help register promotion later.
146-
static bool sinkCondFail(SILLoop *Loop) {
147-
// Only handle innermost loops for now.
148-
if (!Loop->getSubLoops().empty())
149-
return false;
150-
151-
bool Changed = false;
152-
for (auto *BB : Loop->getBlocks()) {
153-
// A list of CondFails that can be moved down.
154-
SmallVector<CondFailInst*, 4> CFs;
155-
// A pair of load and store that are independent of the CondFails and
156-
// can potentially access the same memory.
157-
LoadInst *LIOfPair = nullptr;
158-
bool foundPair = false;
159-
160-
for (auto &Inst : *BB) {
161-
if (foundPair) {
162-
// Move CFs to right before Inst.
163-
for (unsigned I = 0, E = CFs.size(); I < E; I++) {
164-
DEBUG(llvm::dbgs() << "sinking cond_fail down ");
165-
DEBUG(CFs[I]->dump());
166-
DEBUG(llvm::dbgs() << " before ");
167-
DEBUG(Inst.dump());
168-
CFs[I]->moveBefore(&Inst);
169-
}
170-
Changed = true;
171-
172-
foundPair = false;
173-
LIOfPair = nullptr;
174-
}
175-
176-
if (auto CF = dyn_cast<CondFailInst>(&Inst)) {
177-
CFs.push_back(CF);
178-
} else if (auto LI = dyn_cast<LoadInst>(&Inst)) {
179-
if (addressIndependent(LI->getOperand())) {
180-
LIOfPair = LI;
181-
} else {
182-
CFs.clear();
183-
LIOfPair = nullptr;
184-
}
185-
} else if (auto SI = dyn_cast<StoreInst>(&Inst)) {
186-
if (addressIndependent(SI->getDest())) {
187-
if (LIOfPair &&
188-
addressCanPairUp(SI->getDest(), LIOfPair->getOperand()))
189-
foundPair = true;
190-
} else {
191-
CFs.clear();
192-
LIOfPair = nullptr;
193-
}
194-
} else if (Inst.mayHaveSideEffects()) {
195-
CFs.clear();
196-
LIOfPair = nullptr;
197-
}
198-
}
199-
}
200-
return Changed;
201-
}
202-
203128
/// Checks if \p Inst has no side effects which prevent hoisting.
204129
/// The \a SafeReads set contain instructions which we already proved to have
205130
/// no such side effects.
@@ -517,7 +442,6 @@ void LoopTreeOptimization::analyzeCurrentLoop(
517442

518443
void LoopTreeOptimization::optimizeLoop(SILLoop *CurrentLoop,
519444
ReadSet &SafeReads) {
520-
Changed |= sinkCondFail(CurrentLoop);
521445
Changed |= hoistInstructions(CurrentLoop, DomTree, SafeReads,
522446
RunsOnHighLevelSil);
523447
Changed |= sinkFixLifetime(CurrentLoop, DomTree, LoopInfo);

test/SILOptimizer/move_cond_fail.sil

Lines changed: 0 additions & 63 deletions
This file was deleted.

0 commit comments

Comments
 (0)