Skip to content

Commit e3a8a96

Browse files
committed
[OSLogOptimization] Fix the symbolic closure folding code to only create
new closures when all captures of the symbolic closure, including address-typed captures, are symbolic constants. For other closures, they must have been autoclosures passed to the log call and therefore they need not be duplicated but reused as such. This eliminates the need to copy_value the non-constant captured values of closures. Instead the closure itself is copied as a whole. Fixes <rdar://problem/60344043>
1 parent e506e4a commit e3a8a96

File tree

5 files changed

+80
-56
lines changed

5 files changed

+80
-56
lines changed

include/swift/SIL/SILConstants.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -733,6 +733,8 @@ struct SymbolicClosure final
733733
SILType getClosureType() { return closureInst->getType(); }
734734

735735
SubstitutionMap getCallSubstitutionMap() { return substitutionMap; }
736+
737+
bool hasOnlyConstantCaptures() { return !hasNonConstantCaptures; }
736738
};
737739

738740
} // end namespace swift

lib/SIL/SILConstants.cpp

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -763,12 +763,26 @@ SymbolicClosure *SymbolicClosure::create(SILFunction *target,
763763
SingleValueInstruction *closureInst,
764764
SymbolicValueAllocator &allocator) {
765765
// Determine whether there are captured arguments without a symbolic value.
766+
// Consider indirectly captured arguments as well, which can happen with
767+
// @in_guaranteed convention for captures.
766768
bool hasNonConstantCapture = false;
767769
for (SymbolicClosureArgument closureArg : args) {
768770
if (!closureArg.second) {
769771
hasNonConstantCapture = true;
770772
break;
771773
}
774+
SymbolicValue closureValue = closureArg.second.getValue();
775+
// Is capture non-constant?
776+
if (!closureValue.isConstant()) {
777+
hasNonConstantCapture = true;
778+
break;
779+
}
780+
// Is the indirect capture non-constant?
781+
if (closureValue.getKind() == SymbolicValue::Address &&
782+
!closureValue.getAddressValueMemoryObject()->getValue().isConstant()) {
783+
hasNonConstantCapture = true;
784+
break;
785+
}
772786
}
773787

774788
auto byteSizeOfArgs =

lib/SILOptimizer/Mandatory/OSLogOptimization.cpp

Lines changed: 48 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -725,64 +725,66 @@ static SILValue emitCodeForSymbolicValue(SymbolicValue symVal,
725725
assert(expectedType->is<AnyFunctionType>() ||
726726
expectedType->is<SILFunctionType>());
727727

728-
SymbolicClosure *closure = symVal.getClosure();
729-
SubstitutionMap callSubstMap = closure->getCallSubstitutionMap();
730728
SILModule &module = builder.getModule();
731-
ArrayRef<SymbolicClosureArgument> captures = closure->getCaptures();
732-
733-
// Recursively emit code for all captured values that are mapped to a
734-
// symbolic value. If there is a captured value that is not mapped
735-
// to a symbolic value, use the captured value as such (after possibly
736-
// copying non-trivial captures).
737-
SmallVector<SILValue, 4> capturedSILVals;
738-
for (SymbolicClosureArgument capture : captures) {
739-
SILValue captureOperand = capture.first;
740-
Optional<SymbolicValue> captureSymVal = capture.second;
741-
if (!captureSymVal) {
742-
SILFunction &fun = builder.getFunction();
743-
assert(captureOperand->getFunction() == &fun &&
744-
"non-constant captured arugment not defined in this function");
745-
// If the captureOperand is a non-trivial value, it should be copied
746-
// as it now used in a new folded closure.
747-
SILValue captureCopy = makeOwnedCopyOfSILValue(captureOperand, fun);
748-
capturedSILVals.push_back(captureCopy);
749-
continue;
729+
SymbolicClosure *closure = symVal.getClosure();
730+
SILValue resultVal;
731+
if (!closure->hasOnlyConstantCaptures()) {
732+
// If the closure captures a value that is not a constant, it should only
733+
// come from the caller of the log call. Therefore, assert this and reuse
734+
// the closure value.
735+
SingleValueInstruction *originalClosureInst = closure->getClosureInst();
736+
SILFunction &fun = builder.getFunction();
737+
assert(originalClosureInst->getFunction() == &fun &&
738+
"closure with non-constant captures not defined in this function");
739+
// Copy the closure, since the returned value must be owned.
740+
resultVal = makeOwnedCopyOfSILValue(originalClosureInst, fun);
741+
} else {
742+
SubstitutionMap callSubstMap = closure->getCallSubstitutionMap();
743+
ArrayRef<SymbolicClosureArgument> captures = closure->getCaptures();
744+
// Recursively emit code for all captured values which must be mapped to a
745+
// symbolic value.
746+
SmallVector<SILValue, 4> capturedSILVals;
747+
for (SymbolicClosureArgument capture : captures) {
748+
SILValue captureOperand = capture.first;
749+
Optional<SymbolicValue> captureSymVal = capture.second;
750+
assert(captureSymVal);
751+
// Note that the captured operand type may have generic parameters which
752+
// has to be substituted with the substitution map that was inferred by
753+
// the constant evaluator at the partial-apply site.
754+
SILType operandType = captureOperand->getType();
755+
SILType captureType = operandType.subst(module, callSubstMap);
756+
SILValue captureSILVal = emitCodeForSymbolicValue(
757+
captureSymVal.getValue(), captureType.getASTType(), builder, loc,
758+
stringInfo);
759+
capturedSILVals.push_back(captureSILVal);
750760
}
751-
// Here, we have a symbolic value for the capture. Therefore, use it to
752-
// create a new constant at this point. Note that the captured operand
753-
// type may have generic parameters which has to be substituted with the
754-
// substitution map that was inferred by the constant evaluator at the
755-
// partial-apply site.
756-
SILType operandType = captureOperand->getType();
757-
SILType captureType = operandType.subst(module, callSubstMap);
758-
SILValue captureSILVal = emitCodeForSymbolicValue(
759-
captureSymVal.getValue(), captureType.getASTType(), builder, loc,
760-
stringInfo);
761-
capturedSILVals.push_back(captureSILVal);
761+
FunctionRefInst *functionRef =
762+
builder.createFunctionRef(loc, closure->getTarget());
763+
SILType closureType = closure->getClosureType();
764+
ParameterConvention convention =
765+
closureType.getAs<SILFunctionType>()->getCalleeConvention();
766+
resultVal = builder.createPartialApply(loc, functionRef, callSubstMap,
767+
capturedSILVals, convention);
762768
}
763-
764-
FunctionRefInst *functionRef =
765-
builder.createFunctionRef(loc, closure->getTarget());
766-
SILType closureType = closure->getClosureType();
767-
ParameterConvention convention =
768-
closureType.getAs<SILFunctionType>()->getCalleeConvention();
769-
PartialApplyInst *papply = builder.createPartialApply(
770-
loc, functionRef, callSubstMap, capturedSILVals, convention);
771-
// The type of the created closure must be a lowering of the expected type.
772-
auto resultType = papply->getType().castTo<SILFunctionType>();
769+
// If the expected type is a SILFunctionType convert the closure to the
770+
// expected type using a convert_function instruction. Otherwise, if the
771+
// expected type is AnyFunctionType, nothing needs to be done.
772+
// Note that we cannot assert the lowering in the latter case, as that
773+
// utility doesn't exist yet.
774+
auto resultType = resultVal->getType().castTo<SILFunctionType>();
773775
CanType expectedCanType = expectedType->getCanonicalType();
774776
if (auto expectedFnType = dyn_cast<SILFunctionType>(expectedCanType)) {
775777
assert(expectedFnType->getUnsubstitutedType(module)
776778
== resultType->getUnsubstitutedType(module));
777779
// Convert to the expected type if necessary.
778780
if (expectedFnType != resultType) {
779-
auto convert = builder.createConvertFunction(loc, papply,
780-
SILType::getPrimitiveObjectType(expectedFnType),
781-
false);
781+
auto convert = builder.createConvertFunction(
782+
loc, resultVal, SILType::getPrimitiveObjectType(expectedFnType),
783+
false);
782784
return convert;
783785
}
784786
}
785-
return papply;
787+
return resultVal;
786788
}
787789
default: {
788790
llvm_unreachable("Symbolic value kind is not supported");

test/SILOptimizer/OSLogPrototypeCompileTest.sil

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -733,16 +733,15 @@ bb0(%0 : @guaranteed $String):
733733
destroy_value %6 : $OSLogMessageStringCapture
734734
%18 = tuple ()
735735
return %18 : $()
736-
// The first instance of function_ref @idString will be dead code eliminated.
737-
// CHECK: [[ORIGCAPTURE:%[0-9]+]] = copy_value %0 : $String
738-
// CHECK: [[NEWCAPTURE:%[0-9]+]] = copy_value [[ORIGCAPTURE]] : $String
739736
// CHECK: [[FUNREF:%[0-9]+]] = function_ref @idString
740-
// CHECK: [[CLOSURE:%[0-9]+]] = partial_apply [callee_guaranteed] [[FUNREF]]([[NEWCAPTURE]])
741-
// CHECK: [[BORROW:%[0-9]+]] = begin_borrow [[CLOSURE]] : $@callee_guaranteed () -> @owned String
737+
// CHECK: [[ORIGCAPTURE:%[0-9]+]] = copy_value %0 : $String
738+
// CHECK: [[CLOSURE:%[0-9]+]] = partial_apply [callee_guaranteed] [[FUNREF]]([[ORIGCAPTURE]])
739+
// CHECK: [[CLOSURECOPY:%[0-9]+]] = copy_value [[CLOSURE]]
740+
// CHECK: [[BORROW:%[0-9]+]] = begin_borrow [[CLOSURECOPY]] : $@callee_guaranteed () -> @owned String
742741
// CHECK: [[USE:%[0-9]+]] = function_ref @useStringCapture
743742
// CHECK: apply [[USE]]([[BORROW]])
744743
// CHECK: end_borrow [[BORROW]]
745-
// CHECK: destroy_value [[CLOSURE]]
744+
// CHECK: destroy_value [[CLOSURECOPY]]
746745
}
747746

748747
sil [ossa] @genericFunction : $@convention(thin) <U, V> (@in_guaranteed U, @in_guaranteed V) -> Int32 {
@@ -790,11 +789,12 @@ bb0:
790789
// The first instance of function_ref @genericFunction will be dead-code eliminated.
791790
// CHECK: [[FUNREF:%[0-9]+]] = function_ref @genericFunction
792791
// CHECK: [[CLOSURE:%[0-9]+]] = partial_apply [callee_guaranteed] [[FUNREF]]<Int64, Bool>([[CAPTURE1:%[0-9]+]], [[CAPTURE2:%[0-9]+]])
793-
// CHECK: [[BORROW:%[0-9]+]] = begin_borrow [[CLOSURE]] : $@callee_guaranteed () -> Int32
792+
// CHECK: [[CLOSURECOPY:%[0-9]+]] = copy_value [[CLOSURE]]
793+
// CHECK: [[BORROW:%[0-9]+]] = begin_borrow [[CLOSURECOPY]] : $@callee_guaranteed () -> Int32
794794
// CHECK: [[USE:%[0-9]+]] = function_ref @useClosure
795795
// CHECK: apply [[USE]]([[BORROW]])
796796
// CHECK: end_borrow [[BORROW]]
797-
// CHECK: destroy_value [[CLOSURE]]
797+
// CHECK: destroy_value [[CLOSURECOPY]]
798798
}
799799

800800
// Check folding of array of closures. This is essentially the feature needed

test/SILOptimizer/OSLogPrototypeFullOptTest.swift

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,8 +128,10 @@ func testInterpolationWithMultipleArguments(h: Logger) {
128128
@available(OSX 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *)
129129
func testNSObjectInterpolation(h: Logger, nsArray: NSArray) {
130130
h.log("NSArray: \(nsArray, privacy: .public)")
131+
// TODO: check why the ARC optimizer cannot eliminate the many retain/release pairs here.
131132
// CHECK: entry:
132133
// CHECK-NEXT: bitcast %TSo7NSArrayC* %1 to i8*
134+
// CHECK-NEXT: tail call i8* @llvm.objc.retain
133135
// CHECK-NEXT: [[NSARRAY_ARG:%.+]] = tail call i8* @llvm.objc.retain
134136
// CHECK-NEXT: [[LOGLEVEL:%.+]] = tail call swiftcc i8 @"$sSo13os_log_type_ta0A0E7defaultABvgZ"()
135137
// CHECK-NEXT: tail call swiftcc %TSo9OS_os_logC* @"$s14OSLogPrototype6LoggerV9logObjectSo06OS_os_D0Cvg"
@@ -154,18 +156,22 @@ func testNSObjectInterpolation(h: Logger, nsArray: NSArray) {
154156
// CHECK-NEXT: [[OFFSET3:%.+]] = getelementptr inbounds i8, i8* [[BUFFER]], i{{.*}} 3
155157
// CHECK-64-NEXT: store i8 8, i8* [[OFFSET3]], align 1
156158
// CHECK-32-NEXT: store i8 4, i8* [[OFFSET3]], align 1
159+
// CHECK-NEXT: tail call void @llvm.objc.release
157160
// CHECK-NEXT: [[OFFSET4:%.+]] = getelementptr inbounds i8, i8* [[BUFFER]], i{{.*}} 4
158161
// CHECK-NEXT: [[BITCASTED_DEST:%.+]] = bitcast i8* [[OFFSET4]] to %TSo7NSArrayC**
159162
// CHECK-NEXT: [[BITCASTED_SRC:%.+]] = bitcast i8* [[NSARRAY_ARG]] to %TSo7NSArrayC*
160163
// CHECK-NEXT: store %TSo7NSArrayC* [[BITCASTED_SRC]], %TSo7NSArrayC** [[BITCASTED_DEST]], align 1
161164

162-
// TODO: check why the ARC optimizer cannot eliminate this retain/release pair
163165
// CHECK-64-NEXT: tail call void @_os_log_impl({{.*}}, {{.*}} [[LOGOBJ]], i8 zeroext [[LOGLEVEL]], i8* getelementptr inbounds ([20 x i8], [20 x i8]* @{{.*}}, i64 0, i64 0), i8* {{(nonnull )?}}[[BUFFER]], i32 12)
164166
// CHECK-32-NEXT: tail call void @_os_log_impl({{.*}}, {{.*}} [[LOGOBJ]], i8 zeroext [[LOGLEVEL]], i8* getelementptr inbounds ([20 x i8], [20 x i8]* @{{.*}}, i32 0, i32 0), i8* {{(nonnull )?}}[[BUFFER]], i32 8)
165167
// CHECK-NEXT: tail call void @swift_slowDealloc(i8* {{(nonnull )?}}[[BUFFER]]
166-
// CHECK-NEXT: br label %[[NOT_ENABLED]]
168+
// CHECK-NEXT: br label %[[EXIT:[0-9]+]]
167169

168170
// CHECK: [[NOT_ENABLED]]:
171+
// CHECK-NEXT: tail call void @llvm.objc.release
172+
// CHECK-NEXT: br label %[[EXIT]]
173+
174+
// CHECK: [[EXIT]]:
169175
// CHECK-NEXT: tail call void @llvm.objc.release(i8* [[NSARRAY_ARG]])
170176
// CHECK-NEXT: bitcast
171177
// CHECK-NEXT: tail call void @llvm.objc.release

0 commit comments

Comments
 (0)