Skip to content

[Constraint System] Implement heuristics for linked operator expressions in the solver proper. #34399

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.

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 18 commits into from
Nov 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
e0199f2
Add condition for optimizing series of binops that every possible ove…
gregomni Jul 14, 2019
2edba9d
Instead of chaining binops, favor disjunctions with op overloads whos…
gregomni Jul 16, 2019
c84bd00
[ConstraintSystem] Move getResolvedOverloads() from CSStep to Constra…
hborla Oct 14, 2020
b06f749
[ConstraintSystem] Fix build failures and formatting.
hborla Oct 14, 2020
ab6d92b
Favor operators based on existing binds via both basename and type
gregomni Jul 17, 2019
34fa9c2
Merge typevars of operators with shared decls after finding a solutio…
gregomni Jul 18, 2019
cb64d7f
[ConstraintSystem] Fix build failures.
hborla Oct 14, 2020
91291c7
Dump disjunction possibilities on multiple aligned lines for better r…
gregomni Jul 19, 2019
b9e08b2
[ConstraintSystem] Allow the solver to find other solutions after
hborla Oct 16, 2020
8dd2008
[Test] Adjust type checker performance tests
hborla Oct 22, 2020
c9100c4
[ConstraintSystem] Instead of favoring existing operator bindings,
hborla Oct 29, 2020
a8fcdb8
[ConstraintSystem] Remove an unnecessary SIMD special case in
hborla Oct 30, 2020
423d6bd
[ConstraintSystem] Re-instate the operator type variable merging
hborla Oct 30, 2020
0d8a161
[NFC][ConstraintSystem] Update the documentation for
hborla Oct 31, 2020
0995608
[ConstraintSystem] Only factor in existing operator bindings in
hborla Nov 3, 2020
abfc903
[ConstraintSystem] For generic operators, only consider exact decls that
hborla Nov 3, 2020
12dce85
[NFC][ConstraintSystem] Use llvm::count_if for the count methods on
hborla Nov 3, 2020
2507a31
[Test] Add a slow type checker test case from source compatibility su…
hborla Nov 5, 2020
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
15 changes: 9 additions & 6 deletions include/swift/Sema/Constraint.h
Original file line number Diff line number Diff line change
Expand Up @@ -672,13 +672,16 @@ class Constraint final : public llvm::ilist_node<Constraint>,
return Nested;
}

unsigned countActiveNestedConstraints() const {
unsigned count = 0;
for (auto *constraint : Nested)
if (!constraint->isDisabled())
count++;
unsigned countFavoredNestedConstraints() const {
return llvm::count_if(Nested, [](const Constraint *constraint) {
return constraint->isFavored() && !constraint->isDisabled();
});
}

return count;
unsigned countActiveNestedConstraints() const {
return llvm::count_if(Nested, [](const Constraint *constraint) {
return !constraint->isDisabled();
});
}

/// Determine if this constraint represents explicit conversion,
Expand Down
6 changes: 6 additions & 0 deletions include/swift/Sema/ConstraintSystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -5300,6 +5300,12 @@ class ConstraintSystem {
SmallVectorImpl<unsigned> &Ordering,
SmallVectorImpl<unsigned> &PartitionBeginning);

/// The overload sets that have already been resolved along the current path.
const llvm::MapVector<ConstraintLocator *, SelectedOverload> &
getResolvedOverloads() const {
return ResolvedOverloads;
}

/// If we aren't certain that we've emitted a diagnostic, emit a fallback
/// diagnostic.
void maybeProduceFallbackDiagnostic(SolutionApplicationTarget target) const;
Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/CSGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ namespace {
}
};

simplifyBinOpExprTyVars();
simplifyBinOpExprTyVars();

return true;
}
Expand Down
104 changes: 95 additions & 9 deletions lib/Sema/CSSolver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2013,6 +2013,66 @@ static Constraint *tryOptimizeGenericDisjunction(
llvm_unreachable("covered switch");
}

/// Populates the \c found vector with the indices of the given constraints
/// that have a matching type to an existing operator binding elsewhere in
/// the expression.
///
/// Operator bindings that have a matching type to an existing binding
/// are attempted first by the solver because it's very common to chain
/// operators of the same type together.
static void existingOperatorBindingsForDisjunction(ConstraintSystem &CS,
ArrayRef<Constraint *> constraints,
SmallVectorImpl<unsigned> &found) {
auto *choice = constraints.front();
if (choice->getKind() != ConstraintKind::BindOverload)
return;

auto overload = choice->getOverloadChoice();
if (!overload.isDecl())
return;
auto decl = overload.getDecl();
if (!decl->isOperator())
return;

// For concrete operators, consider overloads that have the same type as
// an existing binding, because it's very common to write mixed operator
// expressions where all operands have the same type, e.g. `(x + 10) / 2`.
// For generic operators, only favor an exact overload that has already
// been bound, because mixed operator expressions are far less common, and
// computing generic canonical types is expensive.
SmallSet<CanType, 4> concreteTypesFound;
SmallSet<ValueDecl *, 4> genericDeclsFound;
for (auto overload : CS.getResolvedOverloads()) {
auto resolved = overload.second;
if (!resolved.choice.isDecl())
continue;

auto representativeDecl = resolved.choice.getDecl();
if (!representativeDecl->isOperator())
continue;

auto interfaceType = representativeDecl->getInterfaceType();
if (interfaceType->is<GenericFunctionType>()) {
genericDeclsFound.insert(representativeDecl);
} else {
concreteTypesFound.insert(interfaceType->getCanonicalType());
}
}

for (auto index : indices(constraints)) {
auto *constraint = constraints[index];
if (constraint->isFavored())
continue;

auto *decl = constraint->getOverloadChoice().getDecl();
auto interfaceType = decl->getInterfaceType();
bool isGeneric = interfaceType->is<GenericFunctionType>();
if ((isGeneric && genericDeclsFound.count(decl)) ||
(!isGeneric && concreteTypesFound.count(interfaceType->getCanonicalType())))
found.push_back(index);
}
}

void ConstraintSystem::partitionDisjunction(
ArrayRef<Constraint *> Choices, SmallVectorImpl<unsigned> &Ordering,
SmallVectorImpl<unsigned> &PartitionBeginning) {
Expand Down Expand Up @@ -2042,12 +2102,18 @@ void ConstraintSystem::partitionDisjunction(

// First collect some things that we'll generally put near the beginning or
// end of the partitioning.

SmallVector<unsigned, 4> favored;
SmallVector<unsigned, 4> everythingElse;
SmallVector<unsigned, 4> simdOperators;
SmallVector<unsigned, 4> disabled;
SmallVector<unsigned, 4> unavailable;

// Add existing operator bindings to the main partition first. This often
// helps the solver find a solution fast.
existingOperatorBindingsForDisjunction(*this, Choices, everythingElse);
for (auto index : everythingElse)
taken.insert(Choices[index]);

// First collect disabled and favored constraints.
forEachChoice(Choices, [&](unsigned index, Constraint *constraint) -> bool {
if (constraint->isDisabled()) {
Expand Down Expand Up @@ -2107,7 +2173,6 @@ void ConstraintSystem::partitionDisjunction(
}
};

SmallVector<unsigned, 4> everythingElse;
// Gather the remaining options.
forEachChoice(Choices, [&](unsigned index, Constraint *constraint) -> bool {
everythingElse.push_back(index);
Expand All @@ -2134,13 +2199,34 @@ Constraint *ConstraintSystem::selectDisjunction() {
if (auto *disjunction = selectBestBindingDisjunction(*this, disjunctions))
return disjunction;

// Pick the disjunction with the smallest number of active choices.
auto minDisjunction =
std::min_element(disjunctions.begin(), disjunctions.end(),
[&](Constraint *first, Constraint *second) -> bool {
return first->countActiveNestedConstraints() <
second->countActiveNestedConstraints();
});
// Pick the disjunction with the smallest number of favored, then active choices.
auto cs = this;
auto minDisjunction = std::min_element(disjunctions.begin(), disjunctions.end(),
[&](Constraint *first, Constraint *second) -> bool {
unsigned firstFavored = first->countFavoredNestedConstraints();
unsigned secondFavored = second->countFavoredNestedConstraints();

if (!isOperatorBindOverload(first->getNestedConstraints().front()) ||
!isOperatorBindOverload(second->getNestedConstraints().front()))
return first->countActiveNestedConstraints() < second->countActiveNestedConstraints();

if (firstFavored == secondFavored) {
// Look for additional choices to favor
SmallVector<unsigned, 4> firstExisting;
SmallVector<unsigned, 4> secondExisting;

existingOperatorBindingsForDisjunction(*cs, first->getNestedConstraints(), firstExisting);
firstFavored = firstExisting.size() ? firstExisting.size() : first->countActiveNestedConstraints();
existingOperatorBindingsForDisjunction(*cs, second->getNestedConstraints(), secondExisting);
secondFavored = secondExisting.size() ? secondExisting.size() : second->countActiveNestedConstraints();

return firstFavored < secondFavored;
}

firstFavored = firstFavored ? firstFavored : first->countActiveNestedConstraints();
secondFavored = secondFavored ? secondFavored : second->countActiveNestedConstraints();
return firstFavored < secondFavored;
});

if (minDisjunction != disjunctions.end())
return *minDisjunction;
Expand Down
24 changes: 0 additions & 24 deletions lib/Sema/CSStep.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -614,21 +614,6 @@ bool DisjunctionStep::shortCircuitDisjunctionAt(
Constraint *currentChoice, Constraint *lastSuccessfulChoice) const {
auto &ctx = CS.getASTContext();

// If the successfully applied constraint is favored, we'll consider that to
// be the "best".
if (lastSuccessfulChoice->isFavored() && !currentChoice->isFavored()) {
#if !defined(NDEBUG)
if (lastSuccessfulChoice->getKind() == ConstraintKind::BindOverload) {
auto overloadChoice = lastSuccessfulChoice->getOverloadChoice();
assert((!overloadChoice.isDecl() ||
!overloadChoice.getDecl()->getAttrs().isUnavailable(ctx)) &&
"Unavailable decl should not be favored!");
}
#endif

return true;
}

// Anything without a fix is better than anything with a fix.
if (currentChoice->getFix() && !lastSuccessfulChoice->getFix())
return true;
Expand All @@ -655,15 +640,6 @@ bool DisjunctionStep::shortCircuitDisjunctionAt(
if (currentChoice->getKind() == ConstraintKind::CheckedCast)
return true;

// If we have a SIMD operator, and the prior choice was not a SIMD
// Operator, we're done.
if (currentChoice->getKind() == ConstraintKind::BindOverload &&
isSIMDOperator(currentChoice->getOverloadChoice().getDecl()) &&
lastSuccessfulChoice->getKind() == ConstraintKind::BindOverload &&
!isSIMDOperator(lastSuccessfulChoice->getOverloadChoice().getDecl())) {
return true;
}

return false;
}

Expand Down
9 changes: 2 additions & 7 deletions lib/Sema/CSStep.h
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,6 @@ class SolverStep {
CS.CG.addConstraint(constraint);
}

const llvm::MapVector<ConstraintLocator *, SelectedOverload> &
getResolvedOverloads() const {
return CS.ResolvedOverloads;
}

void recordDisjunctionChoice(ConstraintLocator *disjunctionLocator,
unsigned index) const {
CS.recordDisjunctionChoice(disjunctionLocator, index);
Expand Down Expand Up @@ -716,8 +711,8 @@ class DisjunctionStep final : public BindingStep<DisjunctionChoiceProducer> {
if (!repr || repr == typeVar)
return;

for (auto elt : getResolvedOverloads()) {
auto resolved = elt.second;
for (auto overload : CS.getResolvedOverloads()) {
auto resolved = overload.second;
if (!resolved.boundType->isEqual(repr))
continue;

Expand Down
8 changes: 5 additions & 3 deletions lib/Sema/Constraint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -317,15 +317,17 @@ void Constraint::print(llvm::raw_ostream &Out, SourceManager *sm) const {
Locator->dump(sm, Out);
Out << "]]";
}
Out << ":";
Out << ":\n";

interleave(getNestedConstraints(),
[&](Constraint *constraint) {
if (constraint->isDisabled())
Out << "[disabled] ";
Out << "> [disabled] ";
else
Out << "> ";
constraint->print(Out, sm);
},
[&] { Out << " or "; });
[&] { Out << "\n"; });
return;
}

Expand Down
21 changes: 21 additions & 0 deletions test/Constraints/sr10324.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// RUN: %target-swift-frontend -typecheck -verify %s

// REQUIRES: rdar65007946

struct A {
static func * (lhs: A, rhs: A) -> B { return B() }
static func * (lhs: B, rhs: A) -> B { return B() }
static func * (lhs: A, rhs: B) -> B { return B() }
}
struct B {}

let (x, y, z) = (A(), A(), A())

let w = A() * A() * A() // works

// Should all work
let a = x * y * z
let b = x * (y * z)
let c = (x * y) * z
let d = x * (y * z as B)
let e = (x * y as B) * z
Original file line number Diff line number Diff line change
Expand Up @@ -5,5 +5,4 @@ func test(_ i: Int, _ j: Int) -> Int {
return 1 + (((i >> 1) + (i >> 2) + (i >> 3) + (i >> 4) << 1) << 1) & 0x40 +
1 + (((i >> 1) + (i >> 2) + (i >> 3) + (i >> 4) << 1) << 1) & 0x40 +
1 + (((i >> 1) + (i >> 2) + (i >> 3) + (i >> 4) << 1) << 1) & 0x40
// expected-error@-1 {{the compiler is unable to type-check this expression in reasonable time}}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %scale-test --begin 2 --end 10 --step 2 --select NumConstraintScopes --polynomial-threshold 1.5 %s
// RUN: %scale-test --begin 2 --end 10 --step 2 --select NumConstraintScopes %s
// REQUIRES: asserts,no_asan

let empty: [Int] = []
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,3 @@
// REQUIRES: tools-release,no_asan

_ = [1, 3, 5, 7, 11].filter{ $0 == 1 || $0 == 3 || $0 == 11 || $0 == 1 || $0 == 3 || $0 == 11 } == [ 1, 3, 11 ]
// expected-error@-1 {{unable to type-check}}
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
// RUN: %target-typecheck-verify-swift -solver-expression-time-threshold=1
// REQUIRES: tools-release,no_asan

// expected-error@+1 {{the compiler is unable to type-check this expression in reasonable time}}
let _ = [0].reduce([Int]()) {
return $0.count == 0 && ($1 == 0 || $1 == 2 || $1 == 3) ? [] : $0 + [$1]
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ struct S { var s: String? }

func rdar23861629(_ a: [S]) {
_ = a.reduce("") {
// expected-error@-1 {{reasonable time}}
($0 == "") ? ($1.s ?? "") : ($0 + "," + ($1.s ?? "")) + ($1.s ?? "test") + ($1.s ?? "okay")
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// RUN: %target-typecheck-verify-swift -swift-version 5 -solver-expression-time-threshold=1

func method(_ arg: String, body: () -> [String]) {}

func test(str: String, properties: [String]) {
// expected-error@+1 {{the compiler is unable to type-check this expression in reasonable time}}
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't new, but a case I discovered when debugging the source compat failures. This compiles (very slowly) if you add a type annotation to param below.

method(str + "" + str + "") {
properties.map { param in
"" + param + "" + param + ""
} + [""]
}
}