Skip to content

[OSLogOptimization] Fix folding of symbolic closures to handle @in_guaranteed captures #30386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ravikandhadai merged 1 commit into from
Mar 13, 2020
Merged

[OSLogOptimization] Fix folding of symbolic closures to handle @in_guaranteed captures #30386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/swift/SIL/SILConstants.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,6 +733,8 @@ struct SymbolicClosure final
SILType getClosureType() { return closureInst->getType(); }

SubstitutionMap getCallSubstitutionMap() { return substitutionMap; }

bool hasOnlyConstantCaptures() { return !hasNonConstantCaptures; }
};

} // end namespace swift
Expand Down
14 changes: 14 additions & 0 deletions lib/SIL/SILConstants.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,12 +763,26 @@ SymbolicClosure *SymbolicClosure::create(SILFunction *target,
SingleValueInstruction *closureInst,
SymbolicValueAllocator &allocator) {
// Determine whether there are captured arguments without a symbolic value.
// Consider indirectly captured arguments as well, which can happen with
// @in_guaranteed convention for captures.
bool hasNonConstantCapture = false;
for (SymbolicClosureArgument closureArg : args) {
if (!closureArg.second) {
hasNonConstantCapture = true;
break;
}
SymbolicValue closureValue = closureArg.second.getValue();
// Is capture non-constant?
if (!closureValue.isConstant()) {
hasNonConstantCapture = true;
break;
}
// Is the indirect capture non-constant?
if (closureValue.getKind() == SymbolicValue::Address &&
!closureValue.getAddressValueMemoryObject()->getValue().isConstant()) {
hasNonConstantCapture = true;
break;
}
}

auto byteSizeOfArgs =
Expand Down
94 changes: 48 additions & 46 deletions lib/SILOptimizer/Mandatory/OSLogOptimization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,64 +725,66 @@ static SILValue emitCodeForSymbolicValue(SymbolicValue symVal,
assert(expectedType->is<AnyFunctionType>() ||
expectedType->is<SILFunctionType>());

SymbolicClosure *closure = symVal.getClosure();
SubstitutionMap callSubstMap = closure->getCallSubstitutionMap();
SILModule &module = builder.getModule();
ArrayRef<SymbolicClosureArgument> captures = closure->getCaptures();

// Recursively emit code for all captured values that are mapped to a
// symbolic value. If there is a captured value that is not mapped
// to a symbolic value, use the captured value as such (after possibly
// copying non-trivial captures).
SmallVector<SILValue, 4> capturedSILVals;
for (SymbolicClosureArgument capture : captures) {
SILValue captureOperand = capture.first;
Optional<SymbolicValue> captureSymVal = capture.second;
if (!captureSymVal) {
SILFunction &fun = builder.getFunction();
assert(captureOperand->getFunction() == &fun &&
"non-constant captured arugment not defined in this function");
// If the captureOperand is a non-trivial value, it should be copied
// as it now used in a new folded closure.
SILValue captureCopy = makeOwnedCopyOfSILValue(captureOperand, fun);
capturedSILVals.push_back(captureCopy);
continue;
SymbolicClosure *closure = symVal.getClosure();
SILValue resultVal;
if (!closure->hasOnlyConstantCaptures()) {
// If the closure captures a value that is not a constant, it should only
// come from the caller of the log call. Therefore, assert this and reuse
// the closure value.
SingleValueInstruction *originalClosureInst = closure->getClosureInst();
SILFunction &fun = builder.getFunction();
assert(originalClosureInst->getFunction() == &fun &&
"closure with non-constant captures not defined in this function");
// Copy the closure, since the returned value must be owned.
resultVal = makeOwnedCopyOfSILValue(originalClosureInst, fun);
} else {
SubstitutionMap callSubstMap = closure->getCallSubstitutionMap();
ArrayRef<SymbolicClosureArgument> captures = closure->getCaptures();
// Recursively emit code for all captured values which must be mapped to a
// symbolic value.
SmallVector<SILValue, 4> capturedSILVals;
for (SymbolicClosureArgument capture : captures) {
SILValue captureOperand = capture.first;
Optional<SymbolicValue> captureSymVal = capture.second;
assert(captureSymVal);
// Note that the captured operand type may have generic parameters which
// has to be substituted with the substitution map that was inferred by
// the constant evaluator at the partial-apply site.
SILType operandType = captureOperand->getType();
SILType captureType = operandType.subst(module, callSubstMap);
SILValue captureSILVal = emitCodeForSymbolicValue(
captureSymVal.getValue(), captureType.getASTType(), builder, loc,
stringInfo);
capturedSILVals.push_back(captureSILVal);
}
// Here, we have a symbolic value for the capture. Therefore, use it to
// create a new constant at this point. Note that the captured operand
// type may have generic parameters which has to be substituted with the
// substitution map that was inferred by the constant evaluator at the
// partial-apply site.
SILType operandType = captureOperand->getType();
SILType captureType = operandType.subst(module, callSubstMap);
SILValue captureSILVal = emitCodeForSymbolicValue(
captureSymVal.getValue(), captureType.getASTType(), builder, loc,
stringInfo);
capturedSILVals.push_back(captureSILVal);
FunctionRefInst *functionRef =
builder.createFunctionRef(loc, closure->getTarget());
SILType closureType = closure->getClosureType();
ParameterConvention convention =
closureType.getAs<SILFunctionType>()->getCalleeConvention();
resultVal = builder.createPartialApply(loc, functionRef, callSubstMap,
capturedSILVals, convention);
}

FunctionRefInst *functionRef =
builder.createFunctionRef(loc, closure->getTarget());
SILType closureType = closure->getClosureType();
ParameterConvention convention =
closureType.getAs<SILFunctionType>()->getCalleeConvention();
PartialApplyInst *papply = builder.createPartialApply(
loc, functionRef, callSubstMap, capturedSILVals, convention);
// The type of the created closure must be a lowering of the expected type.
auto resultType = papply->getType().castTo<SILFunctionType>();
// If the expected type is a SILFunctionType convert the closure to the
// expected type using a convert_function instruction. Otherwise, if the
// expected type is AnyFunctionType, nothing needs to be done.
// Note that we cannot assert the lowering in the latter case, as that
// utility doesn't exist yet.
auto resultType = resultVal->getType().castTo<SILFunctionType>();
CanType expectedCanType = expectedType->getCanonicalType();
if (auto expectedFnType = dyn_cast<SILFunctionType>(expectedCanType)) {
assert(expectedFnType->getUnsubstitutedType(module)
== resultType->getUnsubstitutedType(module));
// Convert to the expected type if necessary.
if (expectedFnType != resultType) {
auto convert = builder.createConvertFunction(loc, papply,
SILType::getPrimitiveObjectType(expectedFnType),
false);
auto convert = builder.createConvertFunction(
loc, resultVal, SILType::getPrimitiveObjectType(expectedFnType),
false);
return convert;
}
}
return papply;
return resultVal;
}
default: {
llvm_unreachable("Symbolic value kind is not supported");
Expand Down
16 changes: 8 additions & 8 deletions test/SILOptimizer/OSLogPrototypeCompileTest.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,16 +733,15 @@ bb0(%0 : @guaranteed $String):
destroy_value %6 : $OSLogMessageStringCapture
%18 = tuple ()
return %18 : $()
// The first instance of function_ref @idString will be dead code eliminated.
// CHECK: [[ORIGCAPTURE:%[0-9]+]] = copy_value %0 : $String
// CHECK: [[NEWCAPTURE:%[0-9]+]] = copy_value [[ORIGCAPTURE]] : $String
// CHECK: [[FUNREF:%[0-9]+]] = function_ref @idString
// CHECK: [[CLOSURE:%[0-9]+]] = partial_apply [callee_guaranteed] [[FUNREF]]([[NEWCAPTURE]])
// CHECK: [[BORROW:%[0-9]+]] = begin_borrow [[CLOSURE]] : $@callee_guaranteed () -> @owned String
// CHECK: [[ORIGCAPTURE:%[0-9]+]] = copy_value %0 : $String
// CHECK: [[CLOSURE:%[0-9]+]] = partial_apply [callee_guaranteed] [[FUNREF]]([[ORIGCAPTURE]])
// CHECK: [[CLOSURECOPY:%[0-9]+]] = copy_value [[CLOSURE]]
// CHECK: [[BORROW:%[0-9]+]] = begin_borrow [[CLOSURECOPY]] : $@callee_guaranteed () -> @owned String
// CHECK: [[USE:%[0-9]+]] = function_ref @useStringCapture
// CHECK: apply [[USE]]([[BORROW]])
// CHECK: end_borrow [[BORROW]]
// CHECK: destroy_value [[CLOSURE]]
// CHECK: destroy_value [[CLOSURECOPY]]
}

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

// Check folding of array of closures. This is essentially the feature needed
Expand Down
10 changes: 8 additions & 2 deletions test/SILOptimizer/OSLogPrototypeFullOptTest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,10 @@ func testInterpolationWithMultipleArguments(h: Logger) {
@available(OSX 10.12, iOS 10.0, watchOS 3.0, tvOS 10.0, *)
func testNSObjectInterpolation(h: Logger, nsArray: NSArray) {
h.log("NSArray: \(nsArray, privacy: .public)")
// TODO: check why the ARC optimizer cannot eliminate the many retain/release pairs here.
// CHECK: entry:
// CHECK-NEXT: bitcast %TSo7NSArrayC* %1 to i8*
// CHECK-NEXT: tail call i8* @llvm.objc.retain
// CHECK-NEXT: [[NSARRAY_ARG:%.+]] = tail call i8* @llvm.objc.retain
// CHECK-NEXT: [[LOGLEVEL:%.+]] = tail call swiftcc i8 @"$sSo13os_log_type_ta0A0E7defaultABvgZ"()
// CHECK-NEXT: tail call swiftcc %TSo9OS_os_logC* @"$s14OSLogPrototype6LoggerV9logObjectSo06OS_os_D0Cvg"
Expand All @@ -154,18 +156,22 @@ func testNSObjectInterpolation(h: Logger, nsArray: NSArray) {
// CHECK-NEXT: [[OFFSET3:%.+]] = getelementptr inbounds i8, i8* [[BUFFER]], i{{.*}} 3
// CHECK-64-NEXT: store i8 8, i8* [[OFFSET3]], align 1
// CHECK-32-NEXT: store i8 4, i8* [[OFFSET3]], align 1
// CHECK-NEXT: tail call void @llvm.objc.release
// CHECK-NEXT: [[OFFSET4:%.+]] = getelementptr inbounds i8, i8* [[BUFFER]], i{{.*}} 4
// CHECK-NEXT: [[BITCASTED_DEST:%.+]] = bitcast i8* [[OFFSET4]] to %TSo7NSArrayC**
// CHECK-NEXT: [[BITCASTED_SRC:%.+]] = bitcast i8* [[NSARRAY_ARG]] to %TSo7NSArrayC*
// CHECK-NEXT: store %TSo7NSArrayC* [[BITCASTED_SRC]], %TSo7NSArrayC** [[BITCASTED_DEST]], align 1

// TODO: check why the ARC optimizer cannot eliminate this retain/release pair
// 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)
// 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)
// CHECK-NEXT: tail call void @swift_slowDealloc(i8* {{(nonnull )?}}[[BUFFER]]
// CHECK-NEXT: br label %[[NOT_ENABLED]]
// CHECK-NEXT: br label %[[EXIT:[0-9]+]]

// CHECK: [[NOT_ENABLED]]:
// CHECK-NEXT: tail call void @llvm.objc.release
// CHECK-NEXT: br label %[[EXIT]]

// CHECK: [[EXIT]]:
// CHECK-NEXT: tail call void @llvm.objc.release(i8* [[NSARRAY_ARG]])
// CHECK-NEXT: bitcast
// CHECK-NEXT: tail call void @llvm.objc.release
Expand Down