-
Notifications
You must be signed in to change notification settings - Fork 808
Implementation of GroupSharedLimit to allow increased GroupSharedMemory #7871
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
base: main
Are you sure you want to change the base?
Conversation
|
@JoeCitizen please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
damyanp
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.
Some superficial things that'll need to be addressed at some point, I'm afraid I can't really comment on the actual details of how this works.
I suspect that the whole thing needs to be run through clang-format as well.
| } | ||
| } | ||
|
|
||
| void hlsl::SetShaderProps(PSVRuntimeInfo4 *pInfo4, const DxilModule &DM) { |
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.
| void hlsl::SetShaderProps(PSVRuntimeInfo4 *pInfo4, const DxilModule &DM) { | |
| void hlsl::SetShaderProps(PSVRuntimeInfo4 *Info4, const DxilModule &DM) { |
For new code we generally follow the LLVM Coding Standards. This includes no-p-prefix.
| PSVRuntimeInfo1 *pInfo1, PSVRuntimeInfo2 *pInfo2, | ||
| PSVRuntimeInfo3 *pInfo3, uint8_t ShaderKind, | ||
| const char *EntryName, const char *Comment) { | ||
| PSVRuntimeInfo3 *pInfo3, PSVRuntimeInfo4 *pInfo4, |
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.
IMO in this case following the pInfoN pattern is appropriate.
| if (SpecifiedTGSMSize > 0) { | ||
| MaxSize = SpecifiedTGSMSize; | ||
| } |
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.
| if (SpecifiedTGSMSize > 0) { | |
| MaxSize = SpecifiedTGSMSize; | |
| } | |
| if (SpecifiedTGSMSize > 0) | |
| MaxSize = SpecifiedTGSMSize; |
LLVM coding standards say to omit braces here.
Something's also up with the formatting. Did the format-checker spot it? Anyway, clang-format should fix this for you.
Add a new HLSL attribute for Compute, Amp and Mesh shaders: GroupSharedLimit.
This is used to limit the amount of group shared memory a shader is allowed to statically declare, and validation will fail if the limit is exceeded.
There is no upper limit on this attribute, and it is expected that shader writers set the limit as the lowest common denominator for their target hardware and software use case (typically 48k or 64k for modern GPUs).
If no attribute is declared the existing 32k limit is used to be compatible with existing shaders.
Extends the PSV structures to include the selected limit so that runtime validation can reject the shader if it exceeds the device support.