ConstraintAnalysis: Parse AND operations - #9075
Conversation
| if (auto* nested = unary->value->dynCast<Unary>()) { | ||
| if (Abstract::getUnary(nested->value->type, Abstract::EqZ) == |
There was a problem hiding this comment.
This might be a good place for matches.
| // Negate the entire list of constraints. If we fail to generate something | ||
| // that can be represented as a list of ANDed constraints, the list will be | ||
| // empty (i.e., we can prove nothing). |
There was a problem hiding this comment.
It's not clear to me what the second sentence has to do with negation.
There was a problem hiding this comment.
It just explains what happens when we fail. Negating A && B does not always lead to something that can be represented as C && D - in general it is C || D.
| if (auto* binary = curr->dynCast<Binary>()) { | ||
| // An AND can be recursively processed: both sides must be true. | ||
| if (Abstract::getBinary(binary->left->type, Abstract::And) == | ||
| binary->op) { |
There was a problem hiding this comment.
It might also be nice to use matches here.
| // branch, and use them. | ||
| if (auto branch = getBranchConstraints(block, out); | ||
| branch && checkRelevancy(*branch)) { | ||
| filterRelevant(branch), !branch.empty()) { |
| ConstIterator(const Iterator& other) | ||
| : wasm::ParentIndexIterator<const inplace_vector<T, N>*, ConstIterator>{ | ||
| other.parent, other.index} {} | ||
| ConstIterator(const ConstIterator& other) = default; |
There was a problem hiding this comment.
I can't honestly explain, this is C++ magic beyond my ken...
I was completely stuck on getting this to build, and AI suggested this fix, which works.
There was a problem hiding this comment.
I didn't see new tests for negate. Should there be some?
There was a problem hiding this comment.
The else arms in the lit tests check this. I didn't add unit tests on top of that (seemed excessive?)
|
Match usage added. |
| if (Match::matches(unary->value, | ||
| Match::unary(Abstract::EqZ, Match::local(&get)))) { |
There was a problem hiding this comment.
Best to use using namespace Match; to reduce verbosity here.
There was a problem hiding this comment.
Done, though it is annoying as it shadows other locals here, and doesn't work that cleanly. Still shorter I guess.
| LocalGet* get; | ||
| if (Match::matches(unary->value, | ||
| Match::unary(Abstract::EqZ, Match::local(&get)))) { | ||
| if (matches(u->value, unary(Abstract::EqZ, Match::local(&get)))) { |
There was a problem hiding this comment.
Why it doesn't compile without Match:: in front of local, I couldn't figure out... must be overlapping with some other global namespace name
2 participants
Handles this common code pattern:
We can apply both A and B in the first arm, and
!(A && B)in the second.Also pattern-match
eqz(eqz(..))as that is the only way to represent!= 0in a nested position (otherwise, just
if (local.get)works, which we alreadymatched).