Apply in-place rewrites only if there are no other uses - #304
Closed
simeonschaub wants to merge 2 commits into
Closed
Apply in-place rewrites only if there are no other uses#304simeonschaub wants to merge 2 commits into
simeonschaub wants to merge 2 commits into
Conversation
An inplace=true rewrite rule modifies the matched ops' operands instead of building new instructions. That mutation is only sound when the match is the single user of every op it mutates; any other user silently sees the rewritten value. The one such rule, the nested comparison strength reduction cmpi(addi(a, addi(b, 1)), y, <=) -> cmpi(addi(a, b), y, <), miscompiles kernels in which the 1-based index tile feeds both a mask comparison and a gather or scatter: the shared addi chain is rewritten to 0-based for the comparison while the gather's lowering still subtracts 1 from it, shifting every gathered element down by one. Whether the bug fires depends on worklist order: when the algebra cancellation subi(addi(a, addi(b, c)), c) -> addi(a, b) consumes the gather's subtraction first, the comparison is left as the chain's only user and the mutation happens to be sound. The driver now checks use counts before applying a rule in-place and falls back to the standard mode -- fresh ops, only the root replaced -- when a sub-op that would be mutated is shared. The rewrite fires in both cases; in-place application remains a worklist-efficiency detail. A FileCheck regression test asserts that the strength-reduced mask operand of a masked gather is never decremented again. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
Member
|
Let's just get rid of |
Member
Author
|
Yes, that was my first thought as well, but from reading the comments it looked to me like there were good reasons to use inplace there. If we can get rid of it after all, all the better |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, the inplace rewrite rule would miscompile code like:
since the gather would now see
cafter it has been rewritten