Skip to content

Commit c315bff

Browse files
Merge pull request #30386 from ravikandhadai/handle-in-guaranteed-captures
[OSLogOptimization] Fix folding of symbolic closures to handle @in_guaranteed captures
2 parents 52d3367 + e3a8a96 commit c315bff

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)