triton icon indicating copy to clipboard operation
triton copied to clipboard

Skip values with existing conversions in getConvertBackwardSlice

Open neildhar opened this issue 2 months ago • 6 comments

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.

New contributor declaration

  • [x] I am not making a trivial change, such as fixing a typo in a comment.

  • [x] I have written a PR description following these rules.

  • [x] I have run pre-commit run --from-ref origin/main --to-ref HEAD.

  • Select one of the following.

    • [ ] I have added tests.
      • /test for lit tests
      • /unittest for C++ tests
      • /python/test for end-to-end tests
    • [x] This PR does not need a test because it is a simplification and existing tests pass.
  • Select one of the following.

    • [x] I have not added any lit tests.
    • [ ] The lit tests 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.)

neildhar avatar Sep 25 '25 21:09 neildhar

Can you add a lit test that shows an improvement with this PR?

peterbell10 avatar Oct 02 '25 13:10 peterbell10

@peterbell10 I'm not sure it's possible to observe an improvement from this since the existing conversion is almost always itself a convert_layout. This PR is intended to simplify and document the interface to set up the bug fix in https://github.com/triton-lang/triton/pull/8292.

neildhar avatar Oct 03 '25 20:10 neildhar

@peterbell10 I'm not sure it's possible to observe an improvement from this since the existing conversion is almost always itself a convert_layout. This PR is intended to simplify and document the interface to set up the bug fix in #8292.

are you saying it is an NFC change? If so can you stack the future changes somewhere so that we can review it together?

ThomasRaoux avatar Oct 03 '25 20:10 ThomasRaoux

@ThomasRaoux Yes, I believe the way the pass is currently set up, this doesn't have any effect. My second PR https://github.com/triton-lang/triton/pull/8292 stacks both changes, and includes a TTGIR lit test that crashes before the change, but works after it.

neildhar avatar Oct 03 '25 20:10 neildhar

This was combined with https://github.com/triton-lang/triton/pull/8292 and merged in e3cf2ad0b57604f31e18455794c41772ce31f21d.

neildhar avatar Oct 07 '25 20:10 neildhar

The PR stacked on top of this (https://github.com/triton-lang/triton/pull/8292) was reverted due to regressions. I'm not sure whether the changes in this PR were responsible. If not, @peterbell10, would you consider merging this PR independently?

neildhar avatar Nov 05 '25 16:11 neildhar