Skip to content

Commit 9f85cb8

Browse files
committed
TempRValueElimination: handle potential modifications of the copy-source in a called functions correctly.
This fixes a miscompile in case the source of the optimized copy_addr is modified in a called function with to a not visible alias. This can happen with class properties or global variables. This fix removes the special handling of function parameters, which was just wrong. Instead it simply uses the alias analysis API to check for modifications of the source object. The fix makes TempRValueElimination more conservative and this can cause some performance regressions, but this is unavoidable. rdar://problem/69605657
1 parent d4a6bd3 commit 9f85cb8

File tree

6 files changed

+96
-58
lines changed

6 files changed

+96
-58
lines changed

lib/SILOptimizer/Transforms/TempRValueElimination.cpp

Lines changed: 37 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,8 @@ class TempRValueOptPass : public SILFunctionTransform {
7979
checkTempObjectDestroy(AllocStackInst *tempObj, CopyAddrInst *copyInst,
8080
ValueLifetimeAnalysis::Frontier &tempAddressFrontier);
8181

82-
bool checkNoTempObjectModificationInApply(Operand *tempObjUser,
83-
SILInstruction *inst,
84-
SILValue srcAddr);
82+
bool canApplyBeTreatedAsLoad(Operand *tempObjUser, ApplySite apply,
83+
SILValue srcAddr);
8584

8685
bool tryOptimizeCopyIntoTemp(CopyAddrInst *copyInst);
8786
std::pair<SILBasicBlock::iterator, bool>
@@ -115,57 +114,39 @@ bool TempRValueOptPass::collectLoadsFromProjection(
115114
return true;
116115
}
117116

118-
/// Check if 'tempObjUser' passed to the apply instruction can be modified by it
119-
bool TempRValueOptPass::checkNoTempObjectModificationInApply(
120-
Operand *tempObjUser, SILInstruction *applyInst, SILValue srcAddr) {
121-
ApplySite apply(applyInst);
122-
117+
/// Check if \p tempObjUser, passed to the apply instruction, is only loaded,
118+
/// but not modified and if \p srcAddr is not modified as well.
119+
bool TempRValueOptPass::canApplyBeTreatedAsLoad(
120+
Operand *tempObjUser, ApplySite apply, SILValue srcAddr) {
123121
// Check if the function can just read from tempObjUser.
124122
auto convention = apply.getArgumentConvention(*tempObjUser);
125123
if (!convention.isGuaranteedConvention()) {
126124
LLVM_DEBUG(llvm::dbgs() << " Temp consuming use may write/destroy "
127125
"its source"
128-
<< *applyInst);
129-
return false;
130-
}
131-
132-
// If we do not have an src address, but are indirect, bail. We would need
133-
// to perform function signature specialization to change the functions
134-
// signature to pass something direct.
135-
if (!srcAddr && convention.isIndirectConvention()) {
136-
LLVM_DEBUG(
137-
llvm::dbgs()
138-
<< " Temp used to materialize value for indirect convention?! Can "
139-
"not remove temporary without func sig opts"
140-
<< *applyInst);
126+
<< *apply.getInstruction());
141127
return false;
142128
}
143129

144-
// Check if there is another function argument, which is inout which might
145-
// modify the source address if we have one.
146-
//
147-
// When a use of the temporary is an apply, then we need to prove that the
148-
// function called by the apply cannot modify the temporary's source
149-
// value. By design, this should be handled by
150-
// `checkNoSourceModification`. However, this would be too conservative
151-
// since it's common for the apply to have an @out argument, and alias
152-
// analysis cannot prove that the @out does not alias with `src`. Instead,
153-
// `checkNoSourceModification` always avoids analyzing the current use, so
154-
// applies need to be handled here. We already know that an @out cannot
155-
// alias with `src` because the `src` value must be initialized at the point
156-
// of the call. Hence, it is sufficient to check specifically for another
157-
// @inout that might alias with `src`.
158130
if (srcAddr) {
159-
auto calleeConv = apply.getSubstCalleeConv();
160-
unsigned calleeArgIdx = apply.getCalleeArgIndexOfFirstAppliedArg();
161-
for (const auto &operand : apply.getArgumentOperands()) {
162-
auto argConv = calleeConv.getSILArgumentConvention(calleeArgIdx);
163-
if (argConv.isInoutConvention()) {
164-
if (!aa->isNoAlias(operand.get(), srcAddr)) {
165-
return false;
166-
}
167-
}
168-
++calleeArgIdx;
131+
// If the function may write to the source of the copy_addr, the apply
132+
// cannot be treated as a load: all (potential) writes of the source must
133+
// appear _after_ all loads of the temporary. But in case of a function call
134+
// we don't know in which order the writes and loads are executed inside the
135+
// called function. The source may be written before the temporary is
136+
// loaded, which would make the optization invalid.
137+
if (aa->mayWriteToMemory(apply.getInstruction(), srcAddr))
138+
return false;
139+
} else {
140+
// If we do not have an src address, but are indirect, bail. We would need
141+
// to perform function signature specialization to change the functions
142+
// signature to pass something direct.
143+
if (convention.isIndirectConvention()) {
144+
LLVM_DEBUG(
145+
llvm::dbgs()
146+
<< " Temp used to materialize value for indirect convention?! Can "
147+
"not remove temporary without func sig opts"
148+
<< *apply.getInstruction());
149+
return false;
169150
}
170151
}
171152
return true;
@@ -189,7 +170,8 @@ bool TempRValueOptPass::collectLoads(
189170
SILValue srcAddr, SmallPtrSetImpl<SILInstruction *> &loadInsts) {
190171
// All normal uses (loads) must be in the initialization block.
191172
// (The destroy and dealloc are commonly in a different block though.)
192-
if (user->getParent() != address->getParent())
173+
SILBasicBlock *block = address->getParent();
174+
if (user->getParent() != block)
193175
return false;
194176

195177
// Only allow uses that cannot destroy their operand. We need to be sure
@@ -232,22 +214,25 @@ bool TempRValueOptPass::collectLoads(
232214
LLVM_FALLTHROUGH;
233215
case SILInstructionKind::ApplyInst:
234216
case SILInstructionKind::TryApplyInst: {
235-
if (!checkNoTempObjectModificationInApply(userOp, user, srcAddr))
217+
if (!canApplyBeTreatedAsLoad(userOp, ApplySite(user), srcAddr))
236218
return false;
237219
// Everything is okay with the function call. Register it as a "load".
238220
loadInsts.insert(user);
239221
return true;
240222
}
241223
case SILInstructionKind::BeginApplyInst: {
242-
if (!checkNoTempObjectModificationInApply(userOp, user, srcAddr))
224+
if (!canApplyBeTreatedAsLoad(userOp, ApplySite(user), srcAddr))
243225
return false;
244226

245227
auto beginApply = cast<BeginApplyInst>(user);
246228
// Register 'end_apply'/'abort_apply' as loads as well
247229
// 'checkNoSourceModification' should check instructions until
248230
// 'end_apply'/'abort_apply'.
249-
for (auto tokenUses : beginApply->getTokenResult()->getUses()) {
250-
loadInsts.insert(tokenUses->getUser());
231+
for (auto tokenUse : beginApply->getTokenResult()->getUses()) {
232+
SILInstruction *user = tokenUse->getUser();
233+
if (user->getParent() != block)
234+
return false;
235+
loadInsts.insert(tokenUse->getUser());
251236
}
252237
return true;
253238
}
@@ -285,7 +270,8 @@ bool TempRValueOptPass::collectLoads(
285270
return collectLoadsFromProjection(utedai, srcAddr, loadInsts);
286271
}
287272
case SILInstructionKind::StructElementAddrInst:
288-
case SILInstructionKind::TupleElementAddrInst: {
273+
case SILInstructionKind::TupleElementAddrInst:
274+
case SILInstructionKind::UncheckedAddrCastInst: {
289275
return collectLoadsFromProjection(cast<SingleValueInstruction>(user),
290276
srcAddr, loadInsts);
291277
}

test/SILOptimizer/bridged_casts_folding.swift

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -904,8 +904,12 @@ var anyHashable: AnyHashable = 0
904904

905905
// CHECK-LABEL: $s21bridged_casts_folding29testUncondCastSwiftToSubclassAA08NSObjectI0CyF
906906
// CHECK: [[GLOBAL:%[0-9]+]] = global_addr @$s21bridged_casts_folding11anyHashables03AnyE0Vv
907+
// CHECK: [[TMP:%[0-9]+]] = alloc_stack $AnyHashable
908+
// CHECK: [[ACCESS:%[0-9]+]] = begin_access [read] [static] [no_nested_conflict] [[GLOBAL]]
909+
// CHECK: copy_addr [[ACCESS]] to [initialization] [[TMP]]
907910
// CHECK: [[FUNC:%.*]] = function_ref @$ss11AnyHashableV10FoundationE19_bridgeToObjectiveCSo8NSObjectCyF
908-
// CHECK-NEXT: apply [[FUNC]]([[GLOBAL]])
911+
// CHECK-NEXT: apply [[FUNC]]([[TMP]])
912+
// CHECK-NEXT: destroy_addr [[TMP]]
909913
// CHECK-NEXT: unconditional_checked_cast {{%.*}} : $NSObject to NSObjectSubclass
910914
// CHECK: } // end sil function '$s21bridged_casts_folding29testUncondCastSwiftToSubclassAA08NSObjectI0CyF'
911915
@inline(never)

test/SILOptimizer/opened_archetype_operands_tracking.sil

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -210,12 +210,10 @@ sil hidden_external @use : $@convention(thin) <τ_0_0> (@in τ_0_0) -> ()
210210
// It should contain alloc_ref and alloc_stack instructions using opened archetypes.
211211
// CHECK-LABEL: sil @bar
212212
// CHECK: open_existential{{.*}}C08045E0-2779-11E7-970E-A45E60E99281
213-
// CHECK: alloc_stack{{.*}}C08045E0-2779-11E7-970E-A45E60E99281
214213
// CHECK: alloc_ref{{.*}}C08045E0-2779-11E7-970E-A45E60E99281
215214
// CHECK-NOT: function_ref @use
216215
// CHECK: function_ref @use
217216
// CHECK-NOT: function_ref
218-
// CHECK: dealloc_stack{{.*}}C08045E0-2779-11E7-970E-A45E60E99281
219217
// CHECK: strong_release{{.*}}C08045E0-2779-11E7-970E-A45E60E99281
220218
// CHECK: dealloc_ref{{.*}}C08045E0-2779-11E7-970E-A45E60E99281
221219
// CHECK: end sil function 'bar'

test/SILOptimizer/sil_combine_protocol_conf.swift

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -245,8 +245,10 @@ internal class OtherClass {
245245
// CHECK: load [[S11]]
246246
// CHECK: [[R2:%.*]] = ref_element_addr [[ARG]] : $OtherClass, #OtherClass.arg2
247247
// CHECK: [[O2:%.*]] = open_existential_addr immutable_access [[R2]] : $*GenericPropProtocol to $*@opened("{{.*}}") GenericPropProtocol
248+
// CHECK: [[T1:%.*]] = alloc_stack $@opened
249+
// CHECK: copy_addr [[O2]] to [initialization] [[T1]]
248250
// CHECK: [[W2:%.*]] = witness_method $@opened("{{.*}}") GenericPropProtocol, #GenericPropProtocol.val!getter : <Self where Self : GenericPropProtocol> (Self) -> () -> Int, [[O2]] : $*@opened("{{.*}}") GenericPropProtocol : $@convention(witness_method: GenericPropProtocol) <τ_0_0 where τ_0_0 : GenericPropProtocol> (@in_guaranteed τ_0_0) -> Int
249-
// CHECK: apply [[W2]]<@opened("{{.*}}") GenericPropProtocol>([[O2]]) : $@convention(witness_method: GenericPropProtocol) <τ_0_0 where τ_0_0 : GenericPropProtocol> (@in_guaranteed τ_0_0) -> Int
251+
// CHECK: apply [[W2]]<@opened("{{.*}}") GenericPropProtocol>([[T1]]) : $@convention(witness_method: GenericPropProtocol) <τ_0_0 where τ_0_0 : GenericPropProtocol> (@in_guaranteed τ_0_0) -> Int
250252
// CHECK: struct_extract
251253
// CHECK: integer_literal
252254
// CHECK: builtin
@@ -265,8 +267,10 @@ internal class OtherClass {
265267
// CHECK: cond_fail
266268
// CHECK: [[R5:%.*]] = ref_element_addr [[ARG]] : $OtherClass, #OtherClass.arg4
267269
// CHECK: [[O5:%.*]] = open_existential_addr immutable_access [[R5]] : $*GenericNestedPropProtocol to $*@opened("{{.*}}") GenericNestedPropProtocol
270+
// CHECK: [[T2:%.*]] = alloc_stack $@opened
271+
// CHECK: copy_addr [[O5]] to [initialization] [[T2]]
268272
// CHECK: [[W5:%.*]] = witness_method $@opened("{{.*}}") GenericNestedPropProtocol, #GenericNestedPropProtocol.val!getter : <Self where Self : GenericNestedPropProtocol> (Self) -> () -> Int, [[O5:%.*]] : $*@opened("{{.*}}") GenericNestedPropProtocol : $@convention(witness_method: GenericNestedPropProtocol) <τ_0_0 where τ_0_0 : GenericNestedPropProtocol> (@in_guaranteed τ_0_0) -> Int
269-
// CHECK: apply [[W5]]<@opened("{{.*}}") GenericNestedPropProtocol>([[O5]]) : $@convention(witness_method: GenericNestedPropProtocol) <τ_0_0 where τ_0_0 : GenericNestedPropProtocol> (@in_guaranteed τ_0_0) -> Int
273+
// CHECK: apply [[W5]]<@opened("{{.*}}") GenericNestedPropProtocol>([[T2]]) : $@convention(witness_method: GenericNestedPropProtocol) <τ_0_0 where τ_0_0 : GenericNestedPropProtocol> (@in_guaranteed τ_0_0) -> Int
270274
// CHECK: struct_extract
271275
// CHECK: builtin
272276
// CHECK: tuple_extract
@@ -333,8 +337,10 @@ internal class OtherKlass {
333337
// CHECK: integer_literal
334338
// CHECK: [[R1:%.*]] = ref_element_addr [[ARG]] : $OtherKlass, #OtherKlass.arg2
335339
// CHECK: [[O1:%.*]] = open_existential_addr immutable_access [[R1]] : $*AGenericProtocol to $*@opened("{{.*}}") AGenericProtocol
340+
// CHECK: [[T1:%.*]] = alloc_stack $@opened
341+
// CHECK: copy_addr [[O1]] to [initialization] [[T1]]
336342
// CHECK: [[W1:%.*]] = witness_method $@opened("{{.*}}") AGenericProtocol, #AGenericProtocol.val!getter : <Self where Self : AGenericProtocol> (Self) -> () -> Int, [[O1]] : $*@opened("{{.*}}") AGenericProtocol : $@convention(witness_method: AGenericProtocol) <τ_0_0 where τ_0_0 : AGenericProtocol> (@in_guaranteed τ_0_0) -> Int
337-
// CHECK: apply [[W1]]<@opened("{{.*}}") AGenericProtocol>([[O1]]) : $@convention(witness_method: AGenericProtocol) <τ_0_0 where τ_0_0 : AGenericProtocol> (@in_guaranteed τ_0_0) -> Int
343+
// CHECK: apply [[W1]]<@opened("{{.*}}") AGenericProtocol>([[T1]]) : $@convention(witness_method: AGenericProtocol) <τ_0_0 where τ_0_0 : AGenericProtocol> (@in_guaranteed τ_0_0) -> Int
338344
// CHECK: struct_extract
339345
// CHECK: integer_literal
340346
// CHECK: builtin

test/SILOptimizer/temp_rvalue_opt.sil

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,14 +14,17 @@ struct GS<Base> {
1414
var _value: Builtin.Int64
1515
}
1616

17-
class Klass {}
17+
class Klass {
18+
@_hasStorage var a: String
19+
}
1820

1921
struct Str {
2022
var kl: Klass
2123
var _value: Builtin.Int64
2224
}
2325

2426
sil @unknown : $@convention(thin) () -> ()
27+
sil @load_string : $@convention(thin) (@in_guaranteed String) -> String
2528

2629
sil @inguaranteed_user_without_result : $@convention(thin) (@in_guaranteed Klass) -> () {
2730
bb0(%0 : $*Klass):
@@ -333,6 +336,23 @@ bb0(%0 : $*GS<B>, %1 : $*GS<B>, %2 : $Builtin.Int64):
333336
return %9999 : $()
334337
}
335338

339+
// CHECK-LABEL: sil [ossa] @consider_implicit_aliases_in_callee
340+
// CHECK: alloc_stack
341+
// CHECK-NEXT: copy_addr
342+
// CHECK: } // end sil function 'consider_implicit_aliases_in_callee'
343+
344+
sil [ossa] @consider_implicit_aliases_in_callee : $@convention(thin) (@guaranteed Klass) -> String {
345+
bb0(%0 : @guaranteed $Klass):
346+
%1 = ref_element_addr %0 : $Klass, #Klass.a
347+
%2 = alloc_stack $String
348+
copy_addr %1 to [initialization] %2 : $*String
349+
%f = function_ref @load_string : $@convention(thin) (@in_guaranteed String) -> String
350+
%a = apply %f(%2) : $@convention(thin) (@in_guaranteed String) -> String
351+
destroy_addr %2 : $*String
352+
dealloc_stack %2 : $*String
353+
return %a : $String
354+
}
355+
336356
// Test temp RValue elimination on switches.
337357
// CHECK-LABEL: sil @rvalueSwitch
338358
// CHECK: bb1:
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: %target-build-swift -O %s -o %t/a.out
3+
// RUN: %target-codesign %t/a.out
4+
// RUN: %target-run %t/a.out | %FileCheck %s
5+
6+
// REQUIRES: executable_test
7+
8+
var global: Any = 1
9+
func withValue(action: (Any) -> Void) {
10+
action(global)
11+
}
12+
13+
@inline(never)
14+
public func test_global_side_effect() {
15+
withValue { value in
16+
print(value)
17+
global = 24
18+
print(value)
19+
}
20+
}
21+
22+
// CHECK: 1
23+
// CHECK-NEXT: 1
24+
test_global_side_effect()

0 commit comments

Comments
 (0)