Add ByteAddressBuffer templated load/store tests
#504
Conversation
kmpeng
force-pushed
the
byteaddressbuffer-tests
branch
from
c10505f to
801b497
Compare
lib/API/DX/Device.cpp
Outdated
|
|
||
| static D3D12_SHADER_RESOURCE_VIEW_DESC getSRVDescription(const Resource &R) { | ||
| const uint32_t EltSize = R.getElementSize(); | ||
| const uint32_t EltSize = R.isByteAddressBuffer() ? 4 : R.getElementSize(); |
There was a problem hiding this comment.
Is 4 a magic number? I'm wondering what the significance of 4 is.
There was a problem hiding this comment.
Ah, it looks like this is assuming that a byte address buffer will be using a stride of 4. I'm not sure if that's a good assumption to hardcode?
There was a problem hiding this comment.
Yeah I was wondering if this was the correct approach.
My reason for doing this was the format of ByteAddressBuffer has to be DXGI_FORMAT_R32_TYPELESS regardless of the bit size (otherwise there's an error), and then the element size for that format is always 4 bytes from what I've gathered. I also found that EltSize has to be at least 4 for 16-bit types or we get that the dimensions of the View are invalid. Maybe @bogner has insight on if this approach makes sense?
There was a problem hiding this comment.
I think this assumption is fine, for the reasons you describe, but it'd be nice to make it a bit more obvious where the assumption is coming from here. Would it make sense to have Resource::getElementSize() check if we're a byte address buffer and return 4 in that case? This would give us a single place where we could comment as to why.
On a related note, don't the element sizes in bindSRV() and bindUAV() also need to be updated?
There was a problem hiding this comment.
Yeah that would make more sense. Changed the check to be in Resource::getElementSize() (which also fixes bindSRV() and bindUAV()).
alsepkow
left a comment
alsepkow
left a comment
There was a problem hiding this comment.
Finn/Alex pair review: Left a few minor comments.
|
I have a question for people reviewing this: ByteAddressBuffer load/stores work with arrays in DXC (and explicitly tests them), but we agreed to move towards disallowing array returns in 202x. Should I keep the array test here? And should I implement array support for these methods in Clang? |
We discussed this elsewhere but for posterity I'll note it here: no, we shouldn't test the array behaviour or implement the array behaviour in clang if we're disallowing array returns in 202x. Tests that we reject array versions should be written, but those belong in the clang/dxc repos and don't need to be covered in the offload test suite here. |
lib/API/DX/Device.cpp
Outdated
|
|
||
| static D3D12_SHADER_RESOURCE_VIEW_DESC getSRVDescription(const Resource &R) { | ||
| const uint32_t EltSize = R.getElementSize(); | ||
| const uint32_t EltSize = R.isByteAddressBuffer() ? 4 : R.getElementSize(); |
There was a problem hiding this comment.
I think this assumption is fine, for the reasons you describe, but it'd be nice to make it a bit more obvious where the assumption is coming from here. Would it make sense to have Resource::getElementSize() check if we're a byte address buffer and return 4 in that case? This would give us a single place where we could comment as to why.
On a related note, don't the element sizes in bindSRV() and bindUAV() also need to be updated?
kmpeng
force-pushed
the
byteaddressbuffer-tests
branch
from
106fb3f to
6458811
Compare
3 participants
Closes #266.
Adds tests for ByteAddressBuffer's templated load/store member functions. These test one 32-bit type (uint), bool, one 16-bit type (int16_t), one 64-bit type (int64_t), a struct with an array in it, a small struct, and a big struct. I didn't test every single type of scalar since we're mostly trying to test the various different shapes that could be used.