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.

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

lgc: Move handling of GroupMemcpy for mesh/task shaders to MeshTaskShader #2814

Closed
wants to merge 1 commit into from

Conversation

xazhangAMD
Copy link
Contributor

@xazhangAMD xazhangAMD commented Nov 10, 2023

No description provided.

@xazhangAMD xazhangAMD requested a review from a team as a code owner November 10, 2023 17:47
@xazhangAMD xazhangAMD changed the title lgc: Move handling of GroupMemcpy for mesh/task shaders to MeshTaskSh… lgc: Move handling of GroupMemcpy for mesh/task shaders to MeshTaskShader Nov 10, 2023
@xazhangAMD xazhangAMD force-pushed the group_memcpy_2 branch 4 times, most recently from e5ba891 to e64fcfe Compare November 10, 2023 18:14
@amdvlk-admin
Copy link

Test summary for commit e5ba891

CTS tests (Failed: 0/138362)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35163/69147 (50.9%)
    • Failed: 0/69147 (0.0%)
    • Not Supported: 33984/69147 (49.1%)
    • Warnings: 0/69147 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35242/69215 (50.9%)
    • Failed: 0/69215 (0.0%)
    • Not Supported: 33973/69215 (49.1%)
    • Warnings: 0/69215 (0.0%)

@amdvlk-admin
Copy link

Test summary for commit e64fcfe

CTS tests (Failed: 0/138443)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35211/69228 (50.9%)
    • Failed: 0/69228 (0.0%)
    • Not Supported: 34017/69228 (49.1%)
    • Warnings: 0/69228 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35241/69215 (50.9%)
    • Failed: 0/69215 (0.0%)
    • Not Supported: 33974/69215 (49.1%)
    • Warnings: 0/69215 (0.0%)

@nhaehnle
Copy link
Member

I didn't see the other discussion early enough. Is there really no better way of doing this? For example, if you do move this into its own pass, could use a generic implementation that uses lgc.input.import.builtin or some other pre-existing ops?

I don't particularly fancy a further proliferation of passes, but I think a number of very similar tasks of "lowering operations" like this could be combined into one pass. Though this doesn't necessarily have to happen in this PR.

@xazhangAMD
Copy link
Contributor Author

I didn't see the other discussion early enough. Is there really no better way of doing this? For example, if you do move this into its own pass, could use a generic implementation that uses lgc.input.import.builtin or some other pre-existing ops?

I don't particularly fancy a further proliferation of passes, but I think a number of very similar tasks of "lowering operations" like this could be combined into one pass. Though this doesn't necessarily have to happen in this PR.

Rex is going to refactor this for the CS part. My current change is just to make it a little easier for mesh/tash shaders. BTW, I need another PR first because that solves several random mesh/task shader test failures. 2812

@amdvlk-admin
Copy link

Test summary for commit 98f6e63

CTS tests (Failed: 0/138362)
  • Built with version 1.3.5.2
  • Ubuntu navi3x, Srdcvk
    • Passed: 35163/69147 (50.9%)
    • Failed: 0/69147 (0.0%)
    • Not Supported: 33984/69147 (49.1%)
    • Warnings: 0/69147 (0.0%)
    Ubuntu navi2x, Srdcvk
    • Passed: 35242/69215 (50.9%)
    • Failed: 0/69215 (0.0%)
    • Not Supported: 33973/69215 (49.1%)
    • Warnings: 0/69215 (0.0%)

@amdrexu
Copy link
Contributor

amdrexu commented Nov 11, 2023

Rex is going to refactor this for the CS part.

Yes, I will revisit this.

I don't particularly fancy a further proliferation of passes, but I think a number of very similar tasks of "lowering operations" like this could be combined into one pass.

The pass PatchInitializeWorkgroupMemory does similar thing but is not very generic. I will think about it and do some refactoring work. Currently, task/mesh lowering of memory copy requested by Xavi is added to MeshTaskShader only CS need to be handled particularly.

@nhaehnle
Copy link
Member

So just to be clear, do we wait for the revised version on this?

@xazhangAMD
Copy link
Contributor Author

The fix for random thread IDs has been merged so I have a working version for LLPCFE. This PR can be reviewed as is now or I can also wait for Rex's refactor, too.

@xazhangAMD
Copy link
Contributor Author

Close this PR for now

@xazhangAMD xazhangAMD closed this Jan 3, 2024
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.

4 participants