[ConstraintSystem] Change the order in which we visit disjunctions. #19428
Conversation
Attempt to visit disjunctions that are associated with applies where we have at least some useful information about the types of all of the arguments before visiting other disjunctions. Two tests here got faster, and one slightly slower. One of the faster tests is actually moving from test/ to the slow/ directory in validation-test because despite going from 16s to less than 1s, it was still borderline for what we consider the slow threshold, so I made the test more complex. The one that got a little slower is rdar22022980, which I also made more complex so that it is clearly "slow" by the way we are testing it. slower: rdar22022980.swift faster: rdar33688063.swift expression_too_complex_4.swift
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
lib/Sema/CSSolver.cpp
Outdated
| llvm::SetVector<Constraint *> constraints; | ||
| cs.getConstraintGraph().gatherConstraints( | ||
| rep, constraints, ConstraintGraph::GatheringKind::EquivalenceClass, | ||
| [](Constraint *constraint) { |
There was a problem hiding this comment.
Nit: default callback already does return true; so you don't have to explicitly provide it...
There was a problem hiding this comment.
Ah, thanks for the reminder. I'll remove the body here.
is defualted in the same way.
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
lib/Sema/CSSolver.cpp
Outdated
| // in the given function type. | ||
| static bool haveTypeInformationForAllArguments(AnyFunctionType *fnType, | ||
| ConstraintSystem &cs) { | ||
| for (auto param : fnType->getParams()) |
There was a problem hiding this comment.
This really looks like a great candidate for llvm::all_of...
There was a problem hiding this comment.
Yup, I never went back and cleaned that up after moving the logic into a separate function. I'll push changes to do so.
lib/Sema/CSSolver.cpp
Outdated
| cs.getConstraintGraph().gatherConstraints( | ||
| rep, disjunctions, ConstraintGraph::GatheringKind::EquivalenceClass, | ||
| [](Constraint *match) { | ||
| return match->getKind() == ConstraintKind::Disjunction; |
There was a problem hiding this comment.
It looks like logic from the for loop at the end of this method could be moved into filtering callback, because that test doesn't really require any additional information besides what disjunction has...
There was a problem hiding this comment.
Yeah this normally returns zero or one matches, but you're right I can just fold the logic in. I'll push a change for that as well.
lib/Sema/CSSolver.cpp
Outdated
| static bool haveTypeInformationForAllArguments(AnyFunctionType *fnType, | ||
| ConstraintSystem &cs) { | ||
| for (auto param : fnType->getParams()) | ||
| if (!havePotentialTypesOrLiteralConformances(param.getType(), cs)) |
There was a problem hiding this comment.
Can you use getPlainType() here? getType() wraps the result in an InOutType if its an inout parameter, but is otherwise equivalent. If you need to special-case inouts, just check param.isInOut() instead.
Thanks for using FunctionType::getParams() instead of getInput() though, I appreciate the help in transitioning over to the new representation :-)
There was a problem hiding this comment.
+1, sorry I keep on forgetting that getType() does that... :(
There was a problem hiding this comment.
Sure, I actually strip the InOut and LValue elsewhere so I can just change that to strip the LValue.
I think renaming getType() to getTypeIncludingCruftThatShouldNotBePartOfTheType() and making getPlainType() be getType() would have made it a little easier to not reach for the wrong thing here :).
|
@swift-ci Please smoke test |
|
@swift-ci Please smoke test Linux platform |
|
@swift-ci Please test source compatibility |
…-version 5. This will minimize the chance of breaking code. Currently SwiftLint has one "too complex" expression with this change. Further changes to the solver may improve that situation, and potentially allow us to move this out from -swift-version 5 if we're willing to take the risk of breaking some code as a result.
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
|
@swift-ci Please smoke test |
|
@swift-ci Please test source compatibility |
Attempt to visit disjunctions that are associated with applies where
we have at least some useful information about the types of all of the
arguments before visiting other disjunctions.
Two tests here got faster, and one slightly slower. One of the
faster tests is actually moving from test/ to the slow/ directory in
validation-test because despite going from 16s to less than 1s, it was
still borderline for what we consider the slow threshold, so I made
the test more complex. The one that got a little slower is
rdar22022980, which I also made more complex so that it is clearly
"slow" by the way we are testing it.
slower:
rdar22022980.swift
faster:
rdar33688063.swift
expression_too_complex_4.swift