Conversation
| if (constraints.provesNothing()) { | ||
| setProvesNothing(index); | ||
| } else { | ||
| if (!constraints.provesNothing()) { |
There was a problem hiding this comment.
Drive-by fix, see the comment on line 1054 - we already prove nothing at this point.
| // We just proved we are in unreachable code. | ||
| unreachable = true; | ||
| map.clear(); | ||
| refs.clear(); |
There was a problem hiding this comment.
Another drive-by trivial fix (irrelevent for correctness, see the comment on new line 1215).
|
|
||
| auto& refIndexes = iter->second; | ||
| auto refIndexes = std::move(iter->value); | ||
| refs.erase(iter); |
There was a problem hiding this comment.
(as above, we kept around stale refs unnecessarily; added a comment in the header to mention that this function is called when we wipe out all the info)
| if (map.size() != oldSize) { | ||
| changed = true; | ||
| } |
There was a problem hiding this comment.
We could instead have intersectAndFilter return true if there are any changes it knows about. That might be slightly nicer because it wouldn't be using the size as a side channel for that information.
There was a problem hiding this comment.
Hmm, I kind of disagree. The size isn't a side channel, it is the thing we care about - whether the size changed. Adding code inside intersectAndFilter to track if we remove anything would be more code, and not needed in other callers (though I suppose as this is in a header, that overhead should get optimized out).
There was a problem hiding this comment.
We care about whether there were any changes at all, not about whether the size changed in particular. The only reason checking the size works here is because in-place intersection has the property that the size goes down if the intersection makes any changes.
... but I don't feel strongly about it :)
| // about, propagate it. TODO: even without equality, we can add more | ||
| // constraints here (e.g. x < y and y < 10 can lead to proving x < 10) | ||
| if (auto* other = std::get_if<Index>(&condition.constraint.term)) { | ||
| auto otherConstraints = get(*other); |
There was a problem hiding this comment.
Why did get go away in these lookup sequences?
There was a problem hiding this comment.
get() is a nice "userspace" API that handles missing elements etc., generating an empty AndedConstraintSet if so. It seems nicer and more efficient to access the data structure directly, in the internal methods.
| Index index; | ||
| T value; | ||
|
|
||
| bool operator<(const Indexed& other) const { return index < other.index; } |
There was a problem hiding this comment.
We really should figure out if we can start using <=> yet...
There was a problem hiding this comment.
I thought we checked and we couldn't? Maybe my memory is faulty...
| void insert(Index x) { | ||
| T& insert(T x) { | ||
| if (empty() || back() < x) { | ||
| push_back(std::move(x)); |
There was a problem hiding this comment.
should this be emplace_back so the move constructor gets called, or does it not matter?
There was a problem hiding this comment.
I am no expert but I don't think it matters. Gemini concurs fwiw.
| if (skip > 0) { | ||
| (*this)[i - skip] = std::move((*this)[i]); | ||
| } |
There was a problem hiding this comment.
Does guarding the move behind skip > 0 make a difference for performance? Generally things should be robust against being moved into themselves. Same question below in intersectAndFilter.
| namespace wasm { | ||
|
|
||
| struct SortedVector : public std::vector<Index> { | ||
| template<typename T> struct SortedVector : public std::vector<T> { |
There was a problem hiding this comment.
Semi-related to this PR: I think we shouldn't use public inheritance here but either composition or private inheritance. If a SortedVector is ever passed as a std::vector& then the vector methods will break the invariants of the class (e.g. insert won't preserve the order).
Also deleting a SortedVector via a pointer to std::vector is UB since vector's destructor isn't virtual.
This branch has not been deployed
3 participants
We usually have only a few useful constraints, so the map is mostly
empty. An
unordered_mapis then pretty inefficient, and it is betterto use a vector. Sorting it makes lookup still pretty fast.
This especially helps in ORing two sets of constraints, as we can
just do an intersection (the OR result is only interesting if we had
something in both inputs - otherwise we can prove nothing), which
is efficient on sorted vectors.
To implement this, generalize the existing SortedVector.
This makes the pass 50% faster on average, though the spread
is wide (20%-almost 2X faster). I tested around 10 real-world
wasm files and did not see a single one with less than an 18%
speedup, and never a regression.