Support multiple unbounded arrays within a single SRG #25
Conversation
jeremyong-az
force-pushed
the
Bindless
branch
2 times, most recently
from
9258457
to
894f648
Compare
DX12 binding semantics assign a register to each binding in an array and do not permit overlap. Spaces act as a "namespace" to permit multiple unbounded arrays to be present in a shader. Instead of restricting unbounded array use within an SRG, this change modifies the space assignment strategy. Unbounded arrays (on platforms that require it, DX12 in this case) are designated a unique space, starting from 1000 and counting up. This "spill space" is orthogonal to the space assignment corresponding with the SRG frequency. Signed-off-by: Jeremy Ong <jcong@amazon.com>
jeremyong-az
changed the title
|
A change is needed to correct tests that expect an error with multiple unbounded arrays present. |
There was a problem hiding this comment.
What test cases were added to validate these changes?
https://github.com/o3de/o3de-azslc/wiki/Adding-New-Test-Cases-To-AZSLc
|
@galibzon None as of yet, although a file was added to act as the test once written. The thing that needs to be done first before this PR can be merged is fixing tests that were broken because limits that existed previously are now lifted. |
|
@jeremyong-az , also it's important to highlight that azslc follows the exact same workflow as o3de. o3de/o3de-azslc is upstream, aws-lumberyard-dev/o3de-azslc is origin. So the PR should come from aws-lumberyard-dev/o3de-azslc. |
|
@galibzon I did not have permissions to aws-lumberyard-dev/o3de-azslc when this PR was created. |
|
Let me move this pr to the correct repo, address the failing tests and maybe add a new one to test the new changes. |
| @@ -98,6 +101,10 @@ namespace AZ::ShaderCompiler | |||
| int GetAccumulated(BindingType r) const; | |||
|
|
|||
| int m_space = 0; //<! logical space | |||
|
|
|||
| // See: MultiBindingLocationMarker::m_unboundedSpillSpace | |||
| int m_unboundedSpillSpace; | |||
There was a problem hiding this comment.
Let's initialize this just to be safe.
| MultiBindingLocationMaker(const Options& options, int& unboundedSpillSpace) | ||
| : m_options{ options } | ||
| , m_unboundedSpillSpace{ unboundedSpillSpace } |
There was a problem hiding this comment.
I would prefer unboundedSpillSpace be a pointer instead of a reference, to make it obvious what's going on at the call site. Calling "MultiBindingLocationMaker(options, &unboundedSpillSpaceValue)" is a more clear than calling "MultiBindingLocationMaker(options, unboundedSpillSpaceValue)"
| // We start at an arbitrarily high number to avoid conflicts with spaces occupied by any user declarations | ||
| m_unboundedSpillSpace = 1000; |
There was a problem hiding this comment.
I see that m_unboundedSpillSpace is a mutable member, so it can be assigned here in a const function. Why can't this just be a local variable instead? Is it used after the BuildSignatureDescription function closes? If so, please add a code comment about what's going on, as this is unexpected.
| Texture2D<float4> m_texSRVa[]; // t0+, space1000 | ||
| Texture2D<float4> m_texSRVb[]; // t0+, space1001 | ||
| RWTexture2D<float4> m_texUAVa[]; // u0+, space1002 | ||
| RWTexture2D<float4> m_texUAVb[]; // u0+, space1003 | ||
| Sampler m_samplera[]; // s0+, space1004 | ||
| Sampler m_samplerb[]; // s0+, space1005 | ||
| ConstantBuffer<MyStruct> m_structArraya[]; // b0+, space1006 | ||
| ConstantBuffer<MyStruct> m_structArrayb[]; // b0+, space1007 |
There was a problem hiding this comment.
Should this be an Emission test instead, so we can check the appropriate spaces are used in the generated output?
DX12 binding semantics assign a register to each binding in an array and
do not permit overlap. Spaces act as a "namespace" to permit multiple
unbounded arrays to be present in a shader. Instead of restricting
unbounded array use within an SRG, this change modifies the space
assignment strategy. Unbounded arrays (on platforms that require it,
DX12 in this case) are designated a unique space, starting from 1000 and
counting up. This "spill space" is orthogonal to the space assignment
corresponding with the SRG frequency.