OptimizeInstructions: Add missing invalidation check in consecutive equality test #6596
Conversation
| ;; CHECK-NEXT: (local $3 i32) | ||
| ;; CHECK-NEXT: (drop | ||
| ;; CHECK-NEXT: (block (result (ref $array)) | ||
| ;; CHECK-NEXT: (local.set $0 |
There was a problem hiding this comment.
Why do we set this local if we never use it?
There was a problem hiding this comment.
The utility we use puts all children in locals. After that, they are all either local.gets or constants and we can reason about them easily. It does mean we depend on later passes to clean things up, but it saves complexity (that utility isn't aware of how we will use the children).
| ;; Still identical fallthroughs, but different effects now. | ||
| (drop |
There was a problem hiding this comment.
nit: it would be easier to compare the test input to the output if these were separate functions
There was a problem hiding this comment.
Ah, I actually intentionally grouped them in batches of 4 very similar functions. Is 4 too many, do you think?
There was a problem hiding this comment.
I had to do a bit of scrolling and hunting, but it wasn't so bad. There are certainly also advantages to grouping similar test cases.
There was a problem hiding this comment.
Do you think it's worth splitting them any more, then? I feel it's reasonable but maybe 2 instead of 4 is another way to go here.
2 participants
This existed before #6495 but became noticeable there. We only looked at
the fallthrough values in the later part of
areConsecutiveInputsEqual, butthere can be invalidation due to the non-fallthrough part:
The set can cause the
local.getto differ the second time. To fix this,check if the non-fallthrough part invalidates the fallthrough (but only
on the right hand side).
Fixes #6593