-
Notifications
You must be signed in to change notification settings - Fork 745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OptimizeInstructions: Add missing invalidation check in consecutive equality test #6596
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % question and nit.
;; 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we set this local if we never use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The utility we use puts all children in locals. After that, they are all either local.get
s 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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks.
;; Still identical fallthroughs, but different effects now. | ||
(drop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine as-is.
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.get
to 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