Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[AMD][CanonicalizePtr] Add a series of fixes for the new pipeliner #4743

Merged
merged 4 commits into from
Sep 19, 2024
Merged

[AMD][CanonicalizePtr] Add a series of fixes for the new pipeliner #4743

merged 4 commits into from
Sep 19, 2024

Conversation

giuseros
Copy link
Contributor

This PR is fixing some issues with the CanonicalizePointer pass and the new pipeliner:

  • Don't traverse twice the same nodes
  • Don't assume the operation to delete are in the correct order, but force dropping the reference of the ops before we delete them
  • Add support for select operation (+test), which is used when dealing with multiple buffer (this part has been coauthored with @sjw36 )

sjw36
sjw36 approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@sjw36 sjw36 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Just a couple questions.

third_party/amd/lib/TritonAMDGPUTransforms/CanonicalizePointers.cpp Outdated Show resolved Hide resolved
third_party/amd/lib/TritonAMDGPUTransforms/CanonicalizePointers.cpp Show resolved Hide resolved
@giuseros
Copy link
Contributor Author

@antiagainst , thanks for the review! I think I addressed your comment, PTAL!

@giuseros
Copy link
Contributor Author

Fixed the test. Since we are here, @sjw36 , could you rebase the #4665 and see if there are any more issues?

antiagainst
antiagainst approved these changes Sep 19, 2024
View reviewed changes
Copy link
Collaborator

@antiagainst antiagainst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry final nits..

third_party/amd/lib/TritonAMDGPUTransforms/CanonicalizePointers.cpp Show resolved Hide resolved
third_party/amd/lib/TritonAMDGPUTransforms/CanonicalizePointers.cpp Show resolved Hide resolved
@antiagainst antiagainst merged commit 3ae95a8 into triton-lang:main Sep 19, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antiagainst antiagainst antiagainst approved these changes

@sjw36 sjw36 sjw36 approved these changes

@zhanglx13 zhanglx13 Awaiting requested review from zhanglx13 zhanglx13 is a code owner

@ptillet ptillet Awaiting requested review from ptillet ptillet is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@giuseros @antiagainst @sjw36