Persist valid rematerialisations from slice creation #8292
Merged
Conversation
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
neildhar
force-pushed
the
fix-layout-dominance
branch
from
3ea7fc5 to
7bb6583
Compare
`getConvertBackwardSlice` currently includes any existing rematerialisation it finds in the returned slice. However, this shouldn't be necessary because that value does not need to be rematerialised or included in the cost calculation. Instead, once we find a valid rematerialisation, we can stop the traversal right away.
neildhar
force-pushed
the
fix-layout-dominance
branch
from
7bb6583 to
087c9b7
Compare
7 tasks
peterbell10
reviewed
Oct 3, 2025
| if (Value remat = getRematValue(v, layoutIt->second)) { | ||
| // If we found a valid rematerialization for this value while constructing | ||
| // the slice, use that. | ||
| if (Value remat = existingRemats.lookup({v, layoutIt->second})) { |
There was a problem hiding this comment.
Could you check properlyDominates here instead and not need to pass the additional data structure around everywhere?
There was a problem hiding this comment.
The issue is knowing which users it needs to dominate. We need to consider exactly the same rematerialisations as getConvertBackwardSlice here because any value that getConvertBackwardSlice finds an existing rematerialisation for will not be propagated through when constructing the backward slice.
If we require that the rematerialisation dominate all the users (or the original value itself), then it is possible we will disqualify rematerialisations that getConvertBackwardSlice did allow.
neildhar
force-pushed
the
fix-layout-dominance
branch
from
087c9b7 to
364706a
Compare
When we construct a slice, we correctly only consider rematerialisations that dominate the use in question. However, when rewriting the slice, we allow any rematerialisation, including one that might not dominate the users we want to rewrite. To address this, record the set of rematerialisations that we permitted while constructing the slice, and reuse them when rewriting the slice. This ensures that both operations consider the same set of valid rematerialisations. Note that it is important that they are the same, because slice creation stops once it encounters a valid rematerialisation, so the inputs to the value we are looking up will not be in the slice.
neildhar
force-pushed
the
fix-layout-dominance
branch
from
364706a to
a80b07e
Compare
peterbell10
approved these changes
Oct 7, 2025
|
This is causing some crash in our internal kernels. (causes inconsistent types to be generated: |
ThomasRaoux
added a commit
that referenced
this pull request
Oct 7, 2025
This reverts commit e3cf2ad.
ThomasRaoux
added a commit
that referenced
this pull request
Oct 7, 2025
Reverts #8292 as it causes functional regressions
|
@ThomasRaoux Sorry about the failures, if you have a repro or more information you can share, I'll dig into them when I next get the chance. |
I cannot share the repro at the moment but I'll try to reduce it and share |
yangshuxin
pushed a commit
to yangshuxin/triton
that referenced
this pull request
Oct 9, 2025
When we construct a slice, we correctly only consider rematerialisations that dominate the use in question. However, when rewriting the slice, we allow any rematerialisation, including one that might not dominate the users we want to rewrite. To address this, record the set of rematerialisations that we permitted while constructing the slice, and reuse them when rewriting the slice. This ensures that both operations consider the same set of valid rematerialisations. Note that it is important that they are the same, because slice creation stops once it encounters a valid rematerialisation, so the inputs to the value we are looking up will not be in the slice. This addresses the crash in triton-lang#8257.
yangshuxin
pushed a commit
to yangshuxin/triton
that referenced
this pull request
Oct 9, 2025
…-lang#8395) Reverts triton-lang#8292 as it causes functional regressions
Jokeren
pushed a commit
that referenced
this pull request
Oct 10, 2025
When we construct a slice, we correctly only consider rematerialisations that dominate the use in question. However, when rewriting the slice, we allow any rematerialisation, including one that might not dominate the users we want to rewrite. To address this, record the set of rematerialisations that we permitted while constructing the slice, and reuse them when rewriting the slice. This ensures that both operations consider the same set of valid rematerialisations. Note that it is important that they are the same, because slice creation stops once it encounters a valid rematerialisation, so the inputs to the value we are looking up will not be in the slice. This addresses the crash in #8257.
Jokeren
pushed a commit
that referenced
this pull request
Oct 10, 2025
Reverts #8292 as it causes functional regressions
ita9naiwa
pushed a commit
to ita9naiwa/triton
that referenced
this pull request
Oct 12, 2025
When we construct a slice, we correctly only consider rematerialisations that dominate the use in question. However, when rewriting the slice, we allow any rematerialisation, including one that might not dominate the users we want to rewrite. To address this, record the set of rematerialisations that we permitted while constructing the slice, and reuse them when rewriting the slice. This ensures that both operations consider the same set of valid rematerialisations. Note that it is important that they are the same, because slice creation stops once it encounters a valid rematerialisation, so the inputs to the value we are looking up will not be in the slice. This addresses the crash in triton-lang#8257.
ita9naiwa
pushed a commit
to ita9naiwa/triton
that referenced
this pull request
Oct 12, 2025
…-lang#8395) Reverts triton-lang#8292 as it causes functional regressions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
When we construct a slice, we correctly only consider rematerialisations
that dominate the use in question. However, when rewriting the slice, we
allow any rematerialisation, including one that might not dominate the
users we want to rewrite.
To address this, record the set of rematerialisations that we permitted
while constructing the slice, and reuse them when rewriting the slice.
This ensures that both operations consider the same set of valid
rematerialisations.
Note that it is important that they are the same, because slice creation
stops once it encounters a valid rematerialisation, so the inputs to the
value we are looking up will not be in the slice.
This addresses the crash in #8257.
New contributor declaration
I am not making a trivial change, such as fixing a typo in a comment.
I have written a PR description following these
rules.
I have run
pre-commit run --from-ref origin/main --to-ref HEAD.Select one of the following.
/testforlittests/unittestfor C++ tests/python/testfor end-to-end testsFILL THIS IN.Select one of the following.
littests.littests I have added follow these best practices,including the "tests should be minimal" section. (Usually running Python code
and using the instructions it generates is not minimal.)