ConstraintAnalysis: Copy constraints when copying locals

[kripken]

I believe you suggested this @tlively , and it is necessary to handle real-world code.

[tlively]

It seems we could take everything we know about the current variable, AND it with everything we know about the other variable all at once rather than one constraint at at time (to take advantage of future heuristics about which constraints to keep), then update our knowledge for both the current and other variables to the combined constraint set.

For example, if we know that x <= 10 and y >= 10, then we learn that x == y, then we also know both x == 10 and y == 10.

[tlively]

Oh, but in this case we are also overwriting the current variable, not just learning that it is equal to some other variable, so there is no current knowledge to apply to the other variable.

This would still be an interesting improvement, but this is not the place to handle it. Maybe we can handle both the case handled in this PR and that more general case in a single place where we learn about equality constraints?

[kripken]

Hmm, I'm not sure where a better place would be? As you say, here we are overwriting, so doing a copy here in set() seems right?

[tlively]

BasicBlockConstraintMap::approximateAnd(Index, const Constraint&) (or more accurately, approximateAndInternal) could do this whenever it sees an equality constraint. That will work for this case because BasicBlockConstraintMap::set already calls approximateAndInternal and it will also work for the case of learning an equality constraint without overwriting a variable.

[kripken]

I see, good point. Doing this in approximateAnd would help the non-set case.

I did that in the last commit before the format, but it does turn out a little tricky: we need to handle recursion and some other things. Still, this now handles another case, so I think it's worth it.

[tlively]

We could also test (i32.eq (local.get $x) (local.get $y)) (here and below).

[kripken]

We already have testing of this above (it worked earlier). I can duplicate it here though if you think it helps readability?

[tlively]

Oh no, that's fine, then.

[tlively]

LGTM.

Here's another design that could avoid wasting our limited constraint space with duplicated constraints due to equalities:

• Instead of mapping from local indices to constraint sets, we could could map from local indices to equivalence classes and then map equivalence classes to constraint sets.
• Looking up the constraints for a local would have just one extra step of looking up an equivalence class in the middle.
• Equality constraints between locals would never be stored as contraints; when they are encountered, the two locals' equivalence classes would be merged and the constraint set for the merged equivalence class would be the approximateAnd of the constraints for the two merged classes.
• When a variable is overwritten, it is remapped to a fresh equivalence class.

This scheme would scale much better to programs where several variables are known to be equivalent to each other. We have support/disjoint_sets.h ready to go to represent equivalence classes.

[tlively]

This could be done more directly with map.insert({index, provesNothing}).

[kripken]

Equivalent classes can help, good idea. We already have a utility for that, EquivalentSets (used in SimplifyLocals etc.).

I'll look into it later, not sure how common it is to have equivalent locals in optimized code.

[tlively]

I took a look at EquivalentSets and it looks like the functionality it provides over DisjointSets is the ability to iterate over the elements of the equivalence classes. However, it does not have a merge or union operation for combining equivalence classes, just an add operation that adds a single element to an equivalence class. DisjointSets would be the better match for this use case, where there is no need to iterate over equivalence classes, but there is a need to efficiently merge them (for example when we have a == b and x == y and we learn that a == x).