-
Notifications
You must be signed in to change notification settings - Fork 77
Pre-commit preliminary tests to check for Non-SSA Exec mask instructions #845
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.
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
Pre-commit preliminary tests to check for Non-SSA Exec mask instructions #845
Conversation
|
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 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. |
|
PSDB Build Link: http://mlse-bdc-20dd129:8065/#/builders/10/builds/36 |
llvm/test/CodeGen/AMDGPU/WaveTransform/wavetransform-natural-loops.mir
Outdated
Show resolved
Hide resolved
llvm/test/CodeGen/AMDGPU/WaveTransform/wavetransform-basic-check-rcfg.mir
Outdated
Show resolved
Hide resolved
| @@ -1,19 +1,74 @@ | |||
| # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 6 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
cmc-rep
left a comment
There was a problem hiding this 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
6df5db5 to
7678df0
Compare
|
PSDB Build Link: http://mlse-bdc-20dd129:8065/#/builders/10/builds/38 |
cdevadas
left a comment
There was a problem hiding this 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.
llvm/test/CodeGen/AMDGPU/WaveTransform/wavetransform-natural-loops.mir
Outdated
Show resolved
Hide resolved
|
PSDB Build Link: http://mlse-bdc-20dd129:8065/#/builders/10/builds/39 |
Pre-commit to check for exec mask instruction changes caused by #789