Skip to content

[ConstraintSolver] Avoid unnecessarily increasing score when matching function types #12593

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
merged 1 commit into from
Nov 1, 2017
Merged

[ConstraintSolver] Avoid unnecessarily increasing score when matching function types #12593

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: 0 additions & 2 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1150,8 +1150,6 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,

TypeMatchOptions subflags = getDefaultDecompositionOptions(flags);

increaseScore(ScoreKind::SK_FunctionConversion);

// Add a very narrow exception to SE-0110 by allowing functions that
// take multiple arguments to be passed as an argument in places
// that expect a function that takes a single tuple (of the same
Expand Down
6 changes: 6 additions & 0 deletions test/Constraints/function_conversion.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,9 @@ a(c.d)
func b<T>(_: (T) -> () -> ()) {}

b(c.e)

func bar(_: () -> ()) {}
func bar(_: () throws -> ()) {}
func bar_empty() {}

bar(bar_empty)
12 changes: 12 additions & 0 deletions test/Constraints/rdar35142121.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// RUN: %target-swift-frontend -emit-sil -verify %s | %FileCheck %s

func foo<T>(_ a: T) -> Int {
return 0
}

func foo(_ a: (Int) -> (Int)) -> Int {
return 42
}

// CHECK: function_ref @_T012rdar351421213fooS3icF : $@convention(thin) (@owned @noescape @callee_owned (Int) -> Int) -> Int
let _ = foo({ (a: Int) -> Int in a + 1 })
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rudkx Does this look better?

2 changes: 1 addition & 1 deletion ..._checker_perf/slow/rdar33292740.swift.gyb → ..._checker_perf/fast/rdar33292740.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %scale-test --invert-result --begin 2 --end 7 --step 1 --select incrementScopeCounter %s
// RUN: %scale-test --begin 2 --end 10 --step 1 --select incrementScopeCounter %s
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit surprising that this would have a real effect on whether this is fast or slow. Are you confident this is a real improvement, or was this perhaps not really slow before (e.g. it was slow over that range but wasn't truly exponential?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a real improvement because this triggers logic in the solveSimplified where solver is not going to look at generic overloads if it could find non-generic one with the same score, since we are not increasing the score here some of the cases are going to behave better than before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

// REQUIRES: OS=macosx
// REQUIRES: asserts

Expand Down
2 changes: 1 addition & 1 deletion validation-test/Sema/type_checker_perf/slow/nil_coalescing.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %scale-test --invert-result --begin 1 --end 6 --step 1 --select incrementScopeCounter %s
// RUN: %scale-test --invert-result --begin 1 --end 5 --step 1 --select incrementScopeCounter %s
// REQUIRES: OS=macosx
// REQUIRES: asserts

Expand Down