Skip to content

Commit 1c701b2

Browse files
committed
Simplify SILGen/Condition.
This code was attempting to generate a critical edge on one side of a condition. Not only is that undesirable, but it breaks the abstraction and results an a lot of complexity. As usual, fixing critical edges simply means removing unnecessary complexity.
1 parent f73e6c7 commit 1c701b2

File tree

5 files changed

+65
-156
lines changed

5 files changed

+65
-156
lines changed

lib/SILGen/Condition.cpp

Lines changed: 13 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,9 @@
1919
using namespace swift;
2020
using namespace Lowering;
2121

22-
void Condition::enterTrue(SILGenFunction &SGF) {
23-
assert(TrueBB && "Cannot call enterTrue without a True block!");
24-
25-
// TrueBB has already been inserted somewhere unless there's a
26-
// continuation block.
27-
if (!ContBB) return;
28-
29-
SGF.B.emitBlock(TrueBB);
22+
void Condition::enter(SILGenFunction &SGF, SILBasicBlock *destBB) {
23+
assert(destBB && "Cannot reenter a finished block.");
24+
SGF.B.emitBlock(destBB);
3025
}
3126

3227
/// Extract the last SILLocation used in BB.
@@ -36,110 +31,24 @@ static SILLocation getContinuationLoc(SILBasicBlock &BB, SILLocation Fallback) {
3631
return L;
3732
return Fallback;
3833
}
39-
void Condition::exitTrue(SILGenFunction &SGF, ArrayRef<SILValue> Args) {
40-
// If there's no continuation block, it's because the condition was
41-
// folded to true. In that case, we just continue emitting code as
42-
// if we were still in the true case, and we're unreachable iff the
43-
// end of the true case is unreachable. In other words, there's
44-
// nothing to do.
45-
if (!ContBB) {
46-
assert(!FalseBB && "no continuation");
47-
return;
48-
}
49-
50-
// If there is a continuation block, we should branch to it if the
51-
// current point is reachable.
52-
if (!SGF.B.hasValidInsertionPoint()) {
53-
// If there is no false code, the continuation block has a use
54-
// because the main condition jumps directly to it.
55-
assert(ContBB->pred_empty() || !FalseBB);
34+
void Condition::exit(SILGenFunction &SGF, SILBasicBlock *destBB,
35+
ArrayRef<SILValue> Args) {
36+
// If the current point it reachable, branch to the continuation block.
37+
if (!SGF.B.hasValidInsertionPoint())
5638
return;
57-
}
5839

59-
// Otherwise, resume into the continuation block. This branch might
60-
// be folded by exitFalse if it turns out that that point is
61-
// unreachable.
6240
SGF.B.createBranch(getContinuationLoc(*SGF.B.getInsertionBB(), Loc),
6341
ContBB, Args);
64-
65-
// Coming out of exitTrue, we can be in one of three states:
66-
// - a valid non-terminal IP, but only if there is no continuation
67-
// block, which is only possible if there is no false block;
68-
// - a valid terminal IP, if the end of the true block was reachable; or
69-
// - a cleared IP, if the end of the true block was not reachable.
70-
}
71-
72-
void Condition::enterFalse(SILGenFunction &SGF) {
73-
assert(FalseBB && "entering the false branch when it was not valid");
74-
75-
// FalseBB has already been inserted somewhere unless there's a
76-
// continuation block.
77-
if (!ContBB) return;
78-
79-
// It's possible to have no insertion point here if the end of the
80-
// true case was unreachable.
81-
SGF.B.emitBlock(FalseBB);
82-
}
83-
84-
void Condition::exitFalse(SILGenFunction &SGF, ArrayRef<SILValue> Args) {
85-
// If there's no continuation block, it's because the condition was
86-
// folded to false. In that case, we just continue emitting code as
87-
// if we were still in the false case, and we're unreachable iff the
88-
// end of the false case is unreachable. In other words, there's
89-
// nothing to do.
90-
if (!ContBB) return;
91-
92-
if (ContBB->pred_empty()) {
93-
// If the true case didn't need the continuation block, then
94-
// we don't either, regardless of whether the current location
95-
// is reachable. Just keep inserting / being unreachable
96-
// right where we are.
97-
} else if (!SGF.B.hasValidInsertionPoint()) {
98-
// If the true case did need the continuation block, but the false
99-
// case doesn't, just merge the continuation block back into its
100-
// single predecessor and move the IP there.
101-
//
102-
// Note that doing this tends to strand the false code after
103-
// everything else in the function, so maybe it's not a great idea.
104-
auto PI = ContBB->pred_begin();
105-
SILBasicBlock *ContPred = *PI;
106-
107-
// Verify there was only a single predecessor to ContBB.
108-
++PI;
109-
assert(PI == ContBB->pred_end() && "Only expect one branch to the ContBB");
110-
111-
// Insert before the uncond branch and zap it.
112-
auto *Br = cast<BranchInst>(ContPred->getTerminator());
113-
SGF.B.setInsertionPoint(Br->getParent());
114-
115-
Br->eraseFromParent();
116-
assert(ContBB->pred_empty() &&
117-
"Zapping the branch should make ContBB dead");
118-
} else {
119-
// Otherwise, branch to the continuation block and start inserting there.
120-
SGF.B.createBranch(getContinuationLoc(*SGF.B.getInsertionBB(), Loc),
121-
ContBB, Args);
122-
}
12342
}
12443

12544
SILBasicBlock *Condition::complete(SILGenFunction &SGF) {
126-
// If there is no continuation block, it's because we
127-
// constant-folded the branch. The case-exit will have left us in a
128-
// normal insertion state (i.e. not a post-terminator IP) with
129-
// nothing to clean up after.
130-
if (!ContBB) {
131-
return SGF.B.getInsertionBB();
45+
assert(!TrueBB && "enterTrue is always called.");
46+
if (FalseBB) {
47+
assert(ContBB->getNumArguments() == 0 &&
48+
"block arguments require a non-empty false path.");
49+
SILGenBuilder(SGF.B, FalseBB).createBranch(Loc, ContBB);
50+
FalseBB = nullptr;
13251
}
133-
134-
// Kill the continuation block if it's not being used. Case-exits
135-
// only leave themselves post-terminator if they use the
136-
// continuation block, so we're in an acceptable insertion state.
137-
if (ContBB->pred_empty() && ContBB->args_empty()) {
138-
SGF.eraseBasicBlock(ContBB);
139-
return SGF.B.hasValidInsertionPoint() ? SGF.B.getInsertionBB() : nullptr;
140-
}
141-
142-
// Okay, we need to insert the continuation block.
14352
SGF.B.emitBlock(ContBB);
14453
return ContBB;
14554
}

lib/SILGen/Condition.h

Lines changed: 35 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,12 @@ namespace Lowering {
3232

3333
/// A condition is the result of evaluating a boolean expression as
3434
/// control flow.
35+
///
36+
/// For each Condition instance, `enterTrue` must be called before `complete`.
37+
/// If `enterFalse` is skipped, then an empty fall-through block is created.
3538
class LLVM_LIBRARY_VISIBILITY Condition {
36-
/// The blocks responsible for executing the true and false conditions. A
37-
/// block is non-null if that branch is possible, but it's only an independent
38-
/// block if both branches are possible.
39+
/// The blocks responsible for executing the true and false conditions. These
40+
/// are initialized non-null and set to null after being emitted.
3941
SILBasicBlock *TrueBB;
4042
SILBasicBlock *FalseBB;
4143

@@ -50,30 +52,39 @@ class LLVM_LIBRARY_VISIBILITY Condition {
5052
SILBasicBlock *ContBB,
5153
SILLocation L)
5254
: TrueBB(TrueBB), FalseBB(FalseBB), ContBB(ContBB), Loc(L)
53-
{}
54-
55-
bool hasTrue() const { return TrueBB; }
56-
bool hasFalse() const { return FalseBB; }
57-
58-
/// enterTrue - Begin the emission of the true block. This should only be
59-
/// called if hasTrue() returns true.
60-
void enterTrue(SILGenFunction &SGF);
61-
62-
/// exitTrue - End the emission of the true block. This must be called after
63-
/// enterTrue but before anything else on this Condition.
64-
void exitTrue(SILGenFunction &SGF, ArrayRef<SILValue> Args = {});
65-
66-
/// enterFalse - Begin the emission of the false block. This should only be
67-
/// called if hasFalse() returns true.
68-
void enterFalse(SILGenFunction &SGF);
69-
70-
/// exitFalse - End the emission of the true block. This must be called after
71-
/// enterFalse but before anything else on this Condition.
72-
void exitFalse(SILGenFunction &SGF, ArrayRef<SILValue> Args = {});
73-
55+
{
56+
assert((TrueBB != nullptr && FalseBB != nullptr) &&
57+
"Requires non-null block pointers.");
58+
}
59+
60+
/// enterTrue - Begin the emission of the true block.
61+
void enterTrue(SILGenFunction &SGF) { enter(SGF, TrueBB); }
62+
63+
/// exitTrue - End the emission of the true block.
64+
void exitTrue(SILGenFunction &SGF, ArrayRef<SILValue> Args = {}) {
65+
exit(SGF, TrueBB, Args);
66+
TrueBB = nullptr;
67+
}
68+
69+
/// enterFalse - Begin the emission of the false block.
70+
void enterFalse(SILGenFunction &SGF) { enter(SGF, FalseBB); }
71+
72+
/// exitFalse - End the emission of the true block.
73+
void exitFalse(SILGenFunction &SGF, ArrayRef<SILValue> Args = {}) {
74+
exit(SGF, FalseBB, Args);
75+
FalseBB = nullptr;
76+
}
77+
7478
/// complete - Complete this conditional execution. This should be called
7579
/// only after all other calls on this Condition have been made.
80+
/// This leaves SGF's SILGenBuilder at the continuation block.
7681
SILBasicBlock *complete(SILGenFunction &SGF);
82+
83+
protected:
84+
void enter(SILGenFunction &SGF, SILBasicBlock *destBB);
85+
86+
void exit(SILGenFunction &SGF, SILBasicBlock *destBB,
87+
ArrayRef<SILValue> Args = {});
7788
};
7889

7990
/// A conditional value is one that depends on conditional execution.

lib/SILGen/SILGenExpr.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4436,7 +4436,6 @@ RValue RValueEmitter::visitIfExpr(IfExpr *E, SGFContext C) {
44364436

44374437
// FIXME: We could avoid imploding and reexploding tuples here.
44384438
Condition cond = SGF.emitCondition(E->getCondExpr(),
4439-
/*hasFalse*/ true,
44404439
/*invertCondition*/ false,
44414440
SGF.getLoweredType(E->getType()),
44424441
NumTrueTaken, NumFalseTaken);
@@ -4473,7 +4472,6 @@ RValue RValueEmitter::visitIfExpr(IfExpr *E, SGFContext C) {
44734472
E, lowering.getLoweredType(), C);
44744473

44754474
Condition cond = SGF.emitCondition(E->getCondExpr(),
4476-
/*hasFalse*/ true,
44774475
/*invertCondition*/ false,
44784476
/*contArgs*/ {},
44794477
NumTrueTaken, NumFalseTaken);

lib/SILGen/SILGenFunction.h

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -679,12 +679,10 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
679679
//===--------------------------------------------------------------------===//
680680
// Control flow
681681
//===--------------------------------------------------------------------===//
682-
682+
683683
/// emitCondition - Emit a boolean expression as a control-flow condition.
684684
///
685685
/// \param E - The expression to be evaluated as a condition.
686-
/// \param hasFalseCode - true if the false branch doesn't just lead
687-
/// to the fallthrough.
688686
/// \param invertValue - true if this routine should invert the value before
689687
/// testing true/false.
690688
/// \param contArgs - the types of the arguments to the continuation BB.
@@ -693,14 +691,15 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
693691
/// \param NumTrueTaken - The number of times the condition evaluates to true.
694692
/// \param NumFalseTaken - The number of times the condition evaluates to
695693
/// false.
696-
Condition emitCondition(Expr *E, bool hasFalseCode = true,
697-
bool invertValue = false,
694+
///
695+
/// If `contArgs` is nonempty, then both Condition::exitTrue() and
696+
/// Condition::exitFalse() must be called.
697+
Condition emitCondition(Expr *E, bool invertValue = false,
698698
ArrayRef<SILType> contArgs = {},
699699
ProfileCounter NumTrueTaken = ProfileCounter(),
700700
ProfileCounter NumFalseTaken = ProfileCounter());
701701

702-
Condition emitCondition(SILValue V, SILLocation Loc, bool hasFalseCode = true,
703-
bool invertValue = false,
702+
Condition emitCondition(SILValue V, SILLocation Loc, bool invertValue = false,
704703
ArrayRef<SILType> contArgs = {},
705704
ProfileCounter NumTrueTaken = ProfileCounter(),
706705
ProfileCounter NumFalseTaken = ProfileCounter());
@@ -727,6 +726,9 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
727726
/// section.
728727
SILBasicBlock *createBasicBlock(FunctionSection section);
729728

729+
SILBasicBlock *createBasicBlockAndBranch(SILLocation loc,
730+
SILBasicBlock *destBB);
731+
730732
/// Erase a basic block that was speculatively created and turned
731733
/// out to be unneeded.
732734
///

lib/SILGen/SILGenStmt.cpp

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -146,8 +146,7 @@ static void emitOrDeleteBlock(SILGenFunction &SGF, JumpDest &dest,
146146
SGF.B.emitBlock(BB, BranchLoc);
147147
}
148148

149-
Condition SILGenFunction::emitCondition(Expr *E, bool hasFalseCode,
150-
bool invertValue,
149+
Condition SILGenFunction::emitCondition(Expr *E, bool invertValue,
151150
ArrayRef<SILType> contArgs,
152151
ProfileCounter NumTrueTaken,
153152
ProfileCounter NumFalseTaken) {
@@ -162,12 +161,12 @@ Condition SILGenFunction::emitCondition(Expr *E, bool hasFalseCode,
162161
}
163162
assert(V->getType().castTo<BuiltinIntegerType>()->isFixedWidth(1));
164163

165-
return emitCondition(V, E, hasFalseCode, invertValue, contArgs, NumTrueTaken,
164+
return emitCondition(V, E, invertValue, contArgs, NumTrueTaken,
166165
NumFalseTaken);
167166
}
168167

169168
Condition SILGenFunction::emitCondition(SILValue V, SILLocation Loc,
170-
bool hasFalseCode, bool invertValue,
169+
bool invertValue,
171170
ArrayRef<SILType> contArgs,
172171
ProfileCounter NumTrueTaken,
173172
ProfileCounter NumFalseTaken) {
@@ -179,23 +178,14 @@ Condition SILGenFunction::emitCondition(SILValue V, SILLocation Loc,
179178
for (SILType argTy : contArgs) {
180179
ContBB->createPhiArgument(argTy, ValueOwnershipKind::Owned);
181180
}
182-
183-
SILBasicBlock *FalseBB, *FalseDestBB;
184-
if (hasFalseCode) {
185-
FalseBB = FalseDestBB = createBasicBlock();
186-
} else {
187-
FalseBB = nullptr;
188-
FalseDestBB = ContBB;
189-
}
190181

182+
SILBasicBlock *FalseBB = createBasicBlock();
191183
SILBasicBlock *TrueBB = createBasicBlock();
192184

193185
if (invertValue)
194-
B.createCondBranch(Loc, V, FalseDestBB, TrueBB, NumFalseTaken,
195-
NumTrueTaken);
186+
B.createCondBranch(Loc, V, FalseBB, TrueBB, NumFalseTaken, NumTrueTaken);
196187
else
197-
B.createCondBranch(Loc, V, TrueBB, FalseDestBB, NumTrueTaken,
198-
NumFalseTaken);
188+
B.createCondBranch(Loc, V, TrueBB, FalseBB, NumTrueTaken, NumFalseTaken);
199189

200190
return Condition(TrueBB, FalseBB, ContBB, Loc);
201191
}
@@ -773,7 +763,7 @@ void StmtEmitter::visitRepeatWhileStmt(RepeatWhileStmt *S) {
773763
// to the continuation block.
774764
auto NumTrueTaken = SGF.loadProfilerCount(S->getBody());
775765
auto NumFalseTaken = SGF.loadProfilerCount(S);
776-
Condition Cond = SGF.emitCondition(S->getCond(), /*hasFalseCode*/ false,
766+
Condition Cond = SGF.emitCondition(S->getCond(),
777767
/*invertValue*/ false, /*contArgs*/ {},
778768
NumTrueTaken, NumFalseTaken);
779769

@@ -880,8 +870,7 @@ void StmtEmitter::visitForEachStmt(ForEachStmt *S) {
880870
// condition.
881871
// If it fails, loop around as if 'continue' happened.
882872
if (auto *Where = S->getWhere()) {
883-
auto cond =
884-
SGF.emitCondition(Where, /*hasFalse*/ false, /*invert*/ true);
873+
auto cond = SGF.emitCondition(Where, /*invert*/ true);
885874
// If self is null, branch to the epilog.
886875
cond.enterTrue(SGF);
887876
SGF.Cleanups.emitBranchAndCleanups(loopDest, Where, {});

0 commit comments

Comments
 (0)