Skip to content

Conversation

@lalaniket8
Copy link

Pre-commit to check for exec mask instruction changes caused by #789

@github-actions
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@z1-cciauto
Copy link
Collaborator

@@ -1,19 +1,74 @@
# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 6

Choose a reason for hiding this comment

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

I think this is the good time to modify the original tests to not have PHIs. Most of the tests in this file are initially written by considering the SSA form. We are scheduling this pass in the non-SSA pipeline. In that way, the input to this pass will be in the non-SSA form. Your other patch (#789) should have the getRequiredProperties() to check for setNoPHIs() in the MF properties to ensure the input MF to the wave-transform pass is in the non-SSA form.

Copy link
Author

Choose a reason for hiding this comment

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

We need the tests in this pre-commit to be in SSA, so that it can run with the original WaveTransform pass. Even though the pass is later in the pipeline where PHI-elimination has happened, the pass itself cannot digest nonSSA code given to it.
The support for NonSSA input is added in PR : #789

The same PR should have SSA -> NonSSA changes in the tests.

Copy link

@cmc-rep cmc-rep left a comment

Choose a reason for hiding this comment

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

This looks good to me. This is testing the existing WaveTransform code, which uses SSA. Later on, these tests will be updated (both input and output) with Aniket's code change

@lalaniket8 lalaniket8 force-pushed the amd/dev/lalaniket8/wave-transform-pre-commit-tests branch from 6df5db5 to 7678df0 Compare December 16, 2025 13:08
@z1-cciauto
Copy link
Collaborator

Copy link

@cdevadas cdevadas left a comment

Choose a reason for hiding this comment

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

LGTM except for the suggestions.

@z1-cciauto
Copy link
Collaborator

@lalaniket8 lalaniket8 merged commit 3f8e199 into amd-feature/wave-transform Dec 18, 2025
9 checks passed
@lalaniket8 lalaniket8 deleted the amd/dev/lalaniket8/wave-transform-pre-commit-tests branch December 18, 2025 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants