Skip to content

Commit 02c4ad3

Browse files
varungandhi-applevarungandhi-apple
committed
[SILGen] Branch on result of next() before conversion.
Fixes SR-8688 (rdar://problem/47162940).
1 parent ba78bef commit 02c4ad3

File tree

4 files changed

+55
-41
lines changed

4 files changed

+55
-41
lines changed

‎lib/SILGen/SILGenStmt.cpp

Lines changed: 36 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -969,20 +969,18 @@ void StmtEmitter::visitForEachStmt(ForEachStmt *S) {
969969
// to hold the results. This will be initialized on every entry into the loop
970970
// header and consumed by the loop body. On loop exit, the terminating value
971971
// will be in the buffer.
972-
CanType optTy;
973-
if (S->getConvertElementExpr()) {
974-
optTy = S->getConvertElementExpr()->getType()->getCanonicalType();
975-
} else {
976-
optTy = OptionalType::get(S->getSequenceConformance().getTypeWitnessByName(
977-
S->getSequence()->getType(),
978-
SGF.getASTContext().Id_Element))
979-
->getCanonicalType();
980-
}
972+
CanType optTy =
973+
OptionalType::get(
974+
S->getSequenceConformance().getTypeWitnessByName(
975+
S->getSequence()->getType(), SGF.getASTContext().Id_Element))
976+
->getCanonicalType();
981977
auto &optTL = SGF.getTypeLowering(optTy);
978+
982979
SILValue addrOnlyBuf;
983-
ManagedValue nextBufOrValue;
980+
bool nextResultTyIsAddressOnly =
981+
optTL.isAddressOnly() && SGF.silConv.useLoweredAddresses();
984982

985-
if (optTL.isAddressOnly() && SGF.silConv.useLoweredAddresses())
983+
if (nextResultTyIsAddressOnly)
986984
addrOnlyBuf = SGF.emitTemporaryAllocation(S, optTL.getLoweredType());
987985

988986
// Create a new basic block and jump into it.
@@ -1025,25 +1023,22 @@ void StmtEmitter::visitForEachStmt(ForEachStmt *S) {
10251023
SGFContext().withFollowingSideEffects()));
10261024
};
10271025

1026+
bool hasElementConversion = S->getElementExpr();
10281027
auto buildElementRValue = [&](SILLocation loc, SGFContext ctx) {
10291028
RValue result;
10301029
result = SGF.emitApplyMethod(
10311030
loc, iteratorNextRef, buildArgumentSource(),
10321031
PreparedArguments(ArrayRef<AnyFunctionType::Param>({})),
1033-
S->getElementExpr() ? SGFContext() : ctx);
1034-
if (S->getElementExpr()) {
1035-
SILGenFunction::OpaqueValueRAII pushOpaqueValue(
1036-
SGF, S->getElementExpr(),
1037-
std::move(result).getAsSingleValue(SGF, loc));
1038-
result = SGF.emitRValue(S->getConvertElementExpr(), ctx);
1039-
}
1032+
hasElementConversion ? SGFContext() : ctx);
10401033
return result;
10411034
};
1035+
1036+
ManagedValue nextBufOrElement;
10421037
// Then emit the loop destination block.
10431038
//
10441039
// Advance the generator. Use a scope to ensure that any temporary stack
10451040
// allocations in the subexpression are immediately released.
1046-
if (optTL.isAddressOnly() && SGF.silConv.useLoweredAddresses()) {
1041+
if (nextResultTyIsAddressOnly) {
10471042
// Create the initialization outside of the innerForScope so that the
10481043
// innerForScope doesn't clean it up.
10491044
auto nextInit = SGF.useBufferAsTemporary(addrOnlyBuf, optTL);
@@ -1058,16 +1053,23 @@ void StmtEmitter::visitForEachStmt(ForEachStmt *S) {
10581053
}
10591054
innerForScope.pop();
10601055
}
1061-
nextBufOrValue = nextInit->getManagedAddress();
1056+
nextBufOrElement = nextInit->getManagedAddress();
10621057
} else {
10631058
ArgumentScope innerForScope(SGF, SILLocation(S));
1064-
nextBufOrValue = innerForScope.popPreservingValue(
1059+
nextBufOrElement = innerForScope.popPreservingValue(
10651060
buildElementRValue(SILLocation(S), SGFContext())
10661061
.getAsSingleValue(SGF, SILLocation(S)));
10671062
}
10681063

10691064
SILBasicBlock *failExitingBlock = createBasicBlock();
1070-
SwitchEnumBuilder switchEnumBuilder(SGF.B, S, nextBufOrValue);
1065+
SwitchEnumBuilder switchEnumBuilder(SGF.B, S, nextBufOrElement);
1066+
1067+
auto convertElementRValue = [&](ManagedValue inputValue, SGFContext ctx) -> ManagedValue {
1068+
SILGenFunction::OpaqueValueRAII pushOpaqueValue(SGF, S->getElementExpr(),
1069+
inputValue);
1070+
return SGF.emitRValue(S->getConvertElementExpr(), ctx)
1071+
.getAsSingleValue(SGF, SILLocation(S));
1072+
};
10711073

10721074
switchEnumBuilder.addOptionalSomeCase(
10731075
createBasicBlock(), loopDest.getBlock(),
@@ -1093,13 +1095,22 @@ void StmtEmitter::visitForEachStmt(ForEachStmt *S) {
10931095
//
10941096
// *NOTE* If we do not have an address only value, then inputValue is
10951097
// *already properly unwrapped.
1096-
if (optTL.isAddressOnly() && SGF.silConv.useLoweredAddresses()) {
1098+
SGFContext loopVarCtx{initLoopVars.get()};
1099+
if (nextResultTyIsAddressOnly) {
10971100
inputValue = SGF.emitUncheckedGetOptionalValueFrom(
1098-
S, inputValue, optTL, SGFContext(initLoopVars.get()));
1101+
S, inputValue, optTL,
1102+
hasElementConversion ? SGFContext() : loopVarCtx);
10991103
}
11001104

1105+
CanType optConvertedTy = optTy;
1106+
if (hasElementConversion) {
1107+
inputValue = convertElementRValue(inputValue, loopVarCtx);
1108+
optConvertedTy =
1109+
OptionalType::get(S->getConvertElementExpr()->getType())
1110+
->getCanonicalType();
1111+
}
11011112
if (!inputValue.isInContext())
1102-
RValue(SGF, S, optTy.getOptionalObjectType(), inputValue)
1113+
RValue(SGF, S, optConvertedTy.getOptionalObjectType(), inputValue)
11031114
.forwardInto(SGF, S, initLoopVars.get());
11041115

11051116
// Now that the pattern has been initialized, check any where

‎lib/Sema/CSApply.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8039,13 +8039,13 @@ static Optional<SolutionApplicationTarget> applySolutionToForEachStmt(
80398039
// Convert that Optional<Element> value to the type of the pattern.
80408040
auto optPatternType = OptionalType::get(forEachStmtInfo.initType);
80418041
if (!optPatternType->isEqual(nextResultType)) {
8042-
OpaqueValueExpr *elementExpr =
8043-
new (ctx) OpaqueValueExpr(stmt->getInLoc(), nextResultType,
8044-
/*isPlaceholder=*/true);
8042+
OpaqueValueExpr *elementExpr = new (ctx) OpaqueValueExpr(
8043+
stmt->getInLoc(), nextResultType->getOptionalObjectType(),
8044+
/*isPlaceholder=*/true);
80458045
Expr *convertElementExpr = elementExpr;
80468046
if (TypeChecker::typeCheckExpression(
80478047
convertElementExpr, dc,
8048-
/*contextualInfo=*/{optPatternType, CTP_CoerceOperand})
8048+
/*contextualInfo=*/{forEachStmtInfo.initType, CTP_CoerceOperand})
80498049
.isNull()) {
80508050
return None;
80518051
}

‎test/Profiler/pgo_foreach.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ public func guessForEach1(x: Int32) -> Int32 {
4343
// IR-OPT-LABEL: define swiftcc i32 @$s9pgo_foreach10guessWhiles5Int32VAD1x_tF
4444

4545
public func guessForEach2(x: Int32) -> Int32 {
46-
// SIL: switch_enum {{.*}} : $Optional<(String, Int32)>, case #Optional.some!enumelt: {{.*}} !case_count(168), case #Optional.none!enumelt: {{.*}} !case_count(42)
46+
// SIL: switch_enum {{.*}} : $Optional<(key: String, value: Int32)>, case #Optional.some!enumelt: {{.*}} !case_count(168), case #Optional.none!enumelt: {{.*}} !case_count(42)
4747

4848
var ret : Int32 = 0
4949
let names = ["John" : Int32(1), "Paul" : Int32(2), "George" : Int32(3), "Ringo" : Int32(x)]

‎test/SILGen/foreach.swift

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -604,28 +604,31 @@ func genericFuncWithConversion<T: C>(list : [T]) {
604604
}
605605
}
606606

607-
// FIXME SR-8688: Make sure that branch on result of next() precedes optional injection.
607+
// SR-8688: Check that branch on result of next() precedes optional injection.
608+
// If we branch on the converted result of next(), the loop won't terminate.
608609
//
609610
// CHECK-LABEL: sil hidden [ossa] @$s7foreach32injectForEachElementIntoOptionalyySaySiGF
610611
// CHECK: [[NEXT_RESULT:%.*]] = load [trivial] {{.*}} : $*Optional<Int>
611-
// CHECK: [[PACKED_NEXT_RESULT:%.*]] = enum $Optional<Optional<Int>>, #Optional.some!enumelt, [[NEXT_RESULT]] : $Optional<Int>
612-
// CHECK: switch_enum [[PACKED_NEXT_RESULT]] : $Optional<Optional<Int>>, case #Optional.some!enumelt: [[BB_SOME:bb.*]], case
613-
// CHECK: [[BB_SOME]]([[X_BINDING:%.*]] : $Optional<Int>):
612+
// CHECK: switch_enum [[NEXT_RESULT]] : $Optional<Int>, case #Optional.some!enumelt: [[BB_SOME:bb.*]], case
613+
// CHECK: [[BB_SOME]]([[X_PRE_BINDING:%.*]] : $Int):
614+
// CHECK: [[X_BINDING:%.*]] = enum $Optional<Int>, #Optional.some!enumelt, [[X_PRE_BINDING]] : $Int
614615
// CHECK: debug_value [[X_BINDING]] : $Optional<Int>, let, name "x"
615616
func injectForEachElementIntoOptional(_ xs: [Int]) {
616617
for x : Int? in xs {}
617618
}
618619

619-
// FIXME SR-8688: Make sure that branch on result of next() precedes optional injection.
620+
// SR-8688: Check that branch on result of next() precedes optional injection.
621+
// If we branch on the converted result of next(), the loop won't terminate.
622+
//
620623
// CHECK-LABEL: sil hidden [ossa] @$s7foreach32injectForEachElementIntoOptionalyySayxGlF
621-
// CHECK: [[PACKED_NEXT_RESULT_ADDR:%.*]] = init_enum_data_addr [[PACKED_NEXT_RESULT:%.*]] : $*Optional<Optional<T>>, #Optional.some!enumelt
622-
// CHECK: copy_addr [take] [[NEXT_RESULT:%.*]] to [initialization] [[PACKED_NEXT_RESULT_ADDR:%.*]] : $*Optional<T>
623-
// CHECK: inject_enum_addr [[PACKED_NEXT_RESULT]] : $*Optional<Optional<T>>, #Optional.some!enumelt
624-
// CHECK: switch_enum_addr [[PACKED_NEXT_RESULT]] : $*Optional<Optional<T>>, case #Optional.some!enumelt: [[BB_SOME:bb.*]], case
624+
// CHECK: copy_addr [take] [[NEXT_RESULT:%.*]] to [initialization] [[NEXT_RESULT_COPY:%.*]] : $*Optional<T>
625+
// CHECK: switch_enum_addr [[NEXT_RESULT_COPY]] : $*Optional<T>, case #Optional.some!enumelt: [[BB_SOME:bb.*]], case
625626
// CHECK: [[BB_SOME]]:
626627
// CHECK: [[X_BINDING:%.*]] = alloc_stack $Optional<T>, let, name "x"
627-
// CHECK: [[ADDR:%.*]] = unchecked_take_enum_data_addr [[PACKED_NEXT_RESULT]] : $*Optional<Optional<T>>, #Optional.some!enumelt
628-
// CHECK: copy_addr [take] [[ADDR]] to [initialization] [[X_BINDING]] : $*Optional<T>
628+
// CHECK: [[ADDR:%.*]] = unchecked_take_enum_data_addr [[NEXT_RESULT_COPY]] : $*Optional<T>, #Optional.some!enumelt
629+
// CHECK: [[X_ADDR:%.*]] = init_enum_data_addr [[X_BINDING]] : $*Optional<T>, #Optional.some!enumelt
630+
// CHECK: copy_addr [take] [[ADDR]] to [initialization] [[X_ADDR]] : $*T
631+
// CHECK: inject_enum_addr [[X_BINDING]] : $*Optional<T>, #Optional.some!enumelt
629632
func injectForEachElementIntoOptional<T>(_ xs: [T]) {
630633
for x : T? in xs {}
631634
}

0 commit comments

Comments
 (0)