Skip to content

[ConstraintSolver] When ranking bindings prefer type vars with literal bindings #11966

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

Jump to bottom
Merged
merged 3 commits into from
Sep 18, 2017
Merged

[ConstraintSolver] When ranking bindings prefer type vars with literal bindings #11966

merged 3 commits into from
Sep 18, 2017

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Sep 16, 2017

Change the potential binding ranking logic to prefer type variables
which don't have other type variables related to them, but have literal
bindings, over the ones that do have other type variables.

@slavapestov
Copy link
Contributor

What does this give us?

@xedin
Copy link
Contributor Author

xedin commented Sep 16, 2017 •
edited
Loading

@slavapestov Sometimes we pick something for "best" bindings which isn't really that, e.g. when I remove contraction of BindParam constraint this change makes sure that we don't pick type variable from either side of bind param constraint unless one of the sides is already bound or there is no better literal bindings. That would allow us to remove at least one flag from PotentialBindings.

@xedin xedin force-pushed the prefer-literal-bindings-over-typevars branch from b8ece38 to 603d28d Compare September 16, 2017 01:09
@xedin
Copy link
Contributor Author

xedin commented Sep 16, 2017

@swift-ci please smoke test

@xedin
Copy link
Contributor Author

xedin commented Sep 16, 2017

@swift-ci please smoke test Linux platform

@xedin
Copy link
Contributor Author

xedin commented Sep 16, 2017

@rudkx Looks like it improved one of the slow perf tests too by applying literals earlier \o/

@xedin
Copy link
Contributor Author

xedin commented Sep 16, 2017

@swift-ci please test source compatibility

@xedin
[ConstraintSolver] When ranking bindings prefer type vars with litera…
7d80daf 
…l bindings

Change the potential binding ranking logic to prefer type variables
which don't have other type variables related to them, but have literal
bindings, over the ones that do have other type variables.
@xedin xedin force-pushed the prefer-literal-bindings-over-typevars branch from 603d28d to 7d80daf Compare September 16, 2017 05:20
@xedin xedin requested a review from rudkx September 16, 2017 05:20
@xedin
Copy link
Contributor Author

xedin commented Sep 16, 2017

@rudkx I've added couple more steps to the test which is now much faster and moved it to perf/fast.

@xedin
Copy link
Contributor Author

xedin commented Sep 16, 2017

@swift-ci please smoke test

@xedin xedin changed the title [WIP] [ConstraintSolver] When ranking bindings prefer type vars with literal bindings [ConstraintSolver] When ranking bindings prefer type vars with literal bindings Sep 16, 2017
@xedin
Copy link
Contributor Author

xedin commented Sep 16, 2017

@swift-ci please test source compatibility

@slavapestov
Copy link
Contributor

Awesome!

Although that test case should be diagnosed as invalid :( We don't allow single-element tuples as values anymore.

@xedin
Copy link
Contributor Author

xedin commented Sep 16, 2017

@slavapestov This is intentional, some of the code exhibits exponential behavior when we can't easily find a solution or there is no solution e.g. validation-test/Sema/type_checker_perf/slow/rdar19737632.swift

let a = "a"
let b = "b"
let c = 42
_ = "a=" + a + ";b=" + b + ";c=" + c

Is currently "too complex" because it can't find a solution with favorite types and solver goes on exploring generics forever...

@xedin
[ConstraintSolver] Attempt literal bindings earlier
3792ada 
If the best bindings we could get on the current step are bindings
to literal type without any other type variable involved, let's try
to attempt them right away, that should help to prune search space.
@xedin
[ConstraintSolver] NFC: Refactor PontentialBindings to reference ty…
c8fad86 
…pe variable they belong to
@xedin xedin mentioned this pull request Sep 17, 2017
Closed
@xedin
Copy link
Contributor Author

xedin commented Sep 17, 2017

@rudkx I've added commit which removes restrictions for early attempting of literal bindings and refactored determineBestBindings to use Optional instead of creating empty bindings. Let's see if everything is going to work together :)

@xedin
Copy link
Contributor Author

xedin commented Sep 17, 2017

@swift-ci please smoke test

@xedin
Copy link
Contributor Author

xedin commented Sep 17, 2017

@swift-ci please test source compatibility

rudkx
rudkx approved these changes Sep 18, 2017
View reviewed changes
Copy link
Contributor

@rudkx rudkx left a comment

Choose a reason for hiding this comment

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

LGTM. I had tried the same change to CSSolver.cpp not that long ago but I think something failed. Maybe it also requires the other change to type check one of the expressions or perhaps something else has changed in the meantime.

@xedin
Copy link
Contributor Author

xedin commented Sep 18, 2017

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit 29e69e2 into swiftlang:master Sep 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rudkx rudkx rudkx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xedin @slavapestov @rudkx @swift-ci