diff --git a/include/swift/SIL/SILConstants.h b/include/swift/SIL/SILConstants.h index c360726bd1ecb..4e04f3e8044e7 100644 --- a/include/swift/SIL/SILConstants.h +++ b/include/swift/SIL/SILConstants.h @@ -733,6 +733,8 @@ struct SymbolicClosure final SILType getClosureType() { return closureInst->getType(); } SubstitutionMap getCallSubstitutionMap() { return substitutionMap; } + + bool hasOnlyConstantCaptures() { return !hasNonConstantCaptures; } }; } // end namespace swift diff --git a/lib/SIL/SILConstants.cpp b/lib/SIL/SILConstants.cpp index b0a7e2ff00f68..013d8534e78cc 100644 --- a/lib/SIL/SILConstants.cpp +++ b/lib/SIL/SILConstants.cpp @@ -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 = diff --git a/lib/SILOptimizer/Mandatory/OSLogOptimization.cpp b/lib/SILOptimizer/Mandatory/OSLogOptimization.cpp index 15927b5a4c843..8902267199e4f 100644 --- a/lib/SILOptimizer/Mandatory/OSLogOptimization.cpp +++ b/lib/SILOptimizer/Mandatory/OSLogOptimization.cpp @@ -725,64 +725,66 @@ static SILValue emitCodeForSymbolicValue(SymbolicValue symVal, assert(expectedType->is() || expectedType->is()); - SymbolicClosure *closure = symVal.getClosure(); - SubstitutionMap callSubstMap = closure->getCallSubstitutionMap(); SILModule &module = builder.getModule(); - ArrayRef 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 capturedSILVals; - for (SymbolicClosureArgument capture : captures) { - SILValue captureOperand = capture.first; - Optional 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 captures = closure->getCaptures(); + // Recursively emit code for all captured values which must be mapped to a + // symbolic value. + SmallVector capturedSILVals; + for (SymbolicClosureArgument capture : captures) { + SILValue captureOperand = capture.first; + Optional 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()->getCalleeConvention(); + resultVal = builder.createPartialApply(loc, functionRef, callSubstMap, + capturedSILVals, convention); } - - FunctionRefInst *functionRef = - builder.createFunctionRef(loc, closure->getTarget()); - SILType closureType = closure->getClosureType(); - ParameterConvention convention = - closureType.getAs()->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(); + // 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(); CanType expectedCanType = expectedType->getCanonicalType(); if (auto expectedFnType = dyn_cast(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"); diff --git a/test/SILOptimizer/OSLogPrototypeCompileTest.sil b/test/SILOptimizer/OSLogPrototypeCompileTest.sil index f50dcb7e20548..943f0436d872e 100644 --- a/test/SILOptimizer/OSLogPrototypeCompileTest.sil +++ b/test/SILOptimizer/OSLogPrototypeCompileTest.sil @@ -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) (@in_guaranteed U, @in_guaranteed V) -> Int32 { @@ -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]]([[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 diff --git a/test/SILOptimizer/OSLogPrototypeFullOptTest.swift b/test/SILOptimizer/OSLogPrototypeFullOptTest.swift index aa1b6e4ce247d..3a4eedbcb9832 100644 --- a/test/SILOptimizer/OSLogPrototypeFullOptTest.swift +++ b/test/SILOptimizer/OSLogPrototypeFullOptTest.swift @@ -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" @@ -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