Make areConsecutiveInputsEqual more precise#8783
Conversation
Collect the interfering effects specifically from the expressions that are evaluated in between the LHS and RHS fallthrough values. The previous code overapproximated this by collecting all effects from the LHS and RHS expressions, whether or not the effects were due to subexpressions that would have been evaluated before or after the LHS and RHS values. Also use a direction-aware effect ordering check.
tlively
requested review from
kripken and
stevenfontanella
and removed request for
a team
| } | ||
| if (auto* br = left->dynCast<Break>(); br && br->condition) { | ||
| assert(br->value); | ||
| interferingEffects.walk(br->condition); |
There was a problem hiding this comment.
Why is it safe to skip the br's effects?
There was a problem hiding this comment.
You mean the branch itself? The branch doesn't matter because if control flow branches away and the parent of the consecutive inputs is not executed, then it doesn't matter how we have optimized it.
There was a problem hiding this comment.
It could branch to an intermediate point during the fallthrough. Though as this is a Break, the value is unchanged, unlike a BrOn, so maybe that is safe? But a comment would be good.
There was a problem hiding this comment.
Named blocks do not allow fallthrough, so I don't think this can happen. It wouldn't be safe to allow named blocks to have fallthrough because the branch could come from somewhere inside the final fallthrough value. I can add a comment, though.
There was a problem hiding this comment.
Oh right, we ignore named blocks in fallthrough. I guess this is safe then.
|
Fuzzer just passed 100k iterations on this 🎉 |
| // after it, so we need not look for further effects. But there might be | ||
| // interfering sets in previous br_if conditions, so we cannot just | ||
| // return here. | ||
| if (areMatchingTeeAndGet(left, right)) { |
There was a problem hiding this comment.
Doesn't this have to be checked later, after we computed right's fallthrough?
There was a problem hiding this comment.
Might be simplest to compute right first.
There was a problem hiding this comment.
Yes, we could do even better by checking that right's fallthrough is the matching get. I'll add a TODO to do that in a follow-up. The old code also did not find right's fallthrough first, so this is not a regression.
| (local.tee $temp | ||
| (block (result i32) | ||
| ;; This release store is not ordered before the struct.get, so we know | ||
| ;; it cannot influence the struct.get's value, so we can optimize. |
There was a problem hiding this comment.
| ;; it cannot influence the struct.get's value, so we can optimize. | |
| ;; it cannot influence the struct.get's value, so we can optimize the | |
| ;; cmpxchng to [..] |
2 participants
Collect the interfering effects specifically from the expressions that are evaluated in between the LHS and RHS fallthrough values. The previous code overapproximated this by collecting all effects from the LHS and RHS expressions, whether or not the effects were due to subexpressions that would have been evaluated before or after the LHS and RHS values. Also use a direction-aware effect ordering check.