Skip to content

Conversation

@mgcarrasco
Copy link
Contributor

This PR prevents the mutator from generating illegal memory operations for AMDGCN. In particular, direct store and load instructions on addrspace(8) are not permitted. This PR fixes that by properly introducing casts to addrspace(7) when required.

@jmmartinez jmmartinez requested a review from DataCorrupted June 19, 2025 12:02
@github-actions
Copy link

github-actions bot commented Jun 19, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@mgcarrasco mgcarrasco force-pushed the pr/ir-mutator-addrspace branch from 3b1298e to 67ca2bf Compare June 19, 2025 12:06
@DataCorrupted
Copy link
Member

Can you attach an AMDGCN specific test case? IIUC the isAMDGCN branch you added won't be tested by current test cases at all.

@mgcarrasco
Copy link
Contributor Author

@DataCorrupted thanks for the review. I'll provide the unit test.

@mgcarrasco mgcarrasco force-pushed the pr/ir-mutator-addrspace branch 3 times, most recently from e9b0f59 to 39f8169 Compare June 30, 2025 12:32
@mgcarrasco
Copy link
Contributor Author

mgcarrasco commented Jun 30, 2025

@DataCorrupted thanks again for the feedback. I've added a unit test. We are also planning to incorporate this check into the Verifier in an independent PR. Once that is done, I can also remove the adhoc check for addrspace 8 in a new PR.

@mgcarrasco mgcarrasco force-pushed the pr/ir-mutator-addrspace branch from 39f8169 to ccee029 Compare June 30, 2025 15:52
@mgcarrasco
Copy link
Contributor Author

mgcarrasco commented Jun 30, 2025

The last failure looks unrelated to this PR. I have just rebased to try again.

@mgcarrasco mgcarrasco force-pushed the pr/ir-mutator-addrspace branch from ccee029 to c5b2a5b Compare June 30, 2025 16:07
Copy link
Member

@DataCorrupted DataCorrupted left a comment

Choose a reason for hiding this comment

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

Thanks for the test, only some small changes required.

Copy link
Member

@DataCorrupted DataCorrupted left a comment

Choose a reason for hiding this comment

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

Approved with nit

@mgcarrasco mgcarrasco force-pushed the pr/ir-mutator-addrspace branch from 41550a7 to 0daba8d Compare July 2, 2025 09:19
@mgcarrasco
Copy link
Contributor Author

Thanks @DataCorrupted, I missed those cases.

@mgcarrasco
Copy link
Contributor Author

@DataCorrupted thanks again for your review and feedback. I've addressed the remaining comments. In case the latest changes look good to you, I think the PR could be merged.

@DataCorrupted DataCorrupted merged commit 5224a17 into llvm:main Jul 4, 2025
7 checks passed
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.

2 participants