Add partially mapped resources tests #476
Conversation
include/Support/Pipeline.h
Outdated
| std::optional<VulkanBinding> VKBinding; | ||
| Buffer *BufferPtr = nullptr; | ||
| bool HasCounter; | ||
| int TilesMapped = -1; |
There was a problem hiding this comment.
std::optional<int> would be clearer than using -1 as a sigil here, and isn't really that much more expensive.
|
|
||
|
|
There was a problem hiding this comment.
Some extra whitespace here
| // 32 S structs inside X or Y occupy 64KB of data. (32 * 512 ints * 4 bytes per int) | ||
| // So, any index into the buffer >= [32] will access a new "tile" |
There was a problem hiding this comment.
Looks like this might be over 80-column. It's a bit annoying to clang-format within these .test files (easiest is to paste in another file and clang-format that...), but please try to follow LLVM style regardless.
| memset(ExtraData, 0, CBVSize - R.size() - 1); | ||
| memset(ExtraData, 0, CBVSize - R.size()); |
There was a problem hiding this comment.
Is this an unrelated bug fix? I'm nervous about a change like this without accompanying tests.
There was a problem hiding this comment.
Yes, it is, but I won't be in the business of writing a test for this change here in this PR.
I'll undo this for now.
#477
There was a problem hiding this comment.
Are you still intending to undo this change, or did you find it is actually required?
There was a problem hiding this comment.
I am intending to undo this change, it should already be undone right now.
| Format: Int32 | ||
| Stride: 2048 # S is 512 ints, 512*4 = 2048. | ||
| FillSize: 131072 | ||
| FillValue: 1 |
There was a problem hiding this comment.
It's easier to see what parts of the data are read if we use something more noticeable here (like 42, or 9001 or whatever)
| # XFAIL: Clang | ||
| # XFAIL: Vulkan |
There was a problem hiding this comment.
These need comments like Unimplemented <link to issue> or Bug <link to issue> so that we can track why they're XFAIL'd and loop back appropriately.
bob80905
force-pushed
the
add_unmapped_resources_tests
branch
from
8d9cbbb to
d3cbf91
Compare
lib/API/DX/Device.cpp
Outdated
| // Tile mapping setup (optional if NumTiles > 0) | ||
| const UINT NumTiles = getNumTiles(R.TilesMapped, ResDesc.Width); | ||
|
|
||
| ComPtr<ID3D12Heap> Heap; // optional, only created if NumTiles > 0 | ||
|
|
||
| if (NumTiles > 0) { |
There was a problem hiding this comment.
This whole block (up to the updateTileMappings call) looks identical to the SRV version. Can we pull out a helper function to call in both cases?
There was a problem hiding this comment.
Turns out I'll have to leave unmapped UAVs for another time, so I'll pull out a helper function in a future PR.
lib/API/DX/Device.cpp
Outdated
| // Tile mapping setup (optional if NumTiles > 0) | ||
| const UINT NumTiles = getNumTiles(R.TilesMapped, ResDesc.Width); |
There was a problem hiding this comment.
This comment feels a bit confusing - NumTiles being zero is the edge case given that it can only happen if we explicitly set zero tiles. "optional if NumTiles > 0" sounds to me like NumTiles will usually be zero.
bob80905
force-pushed
the
add_unmapped_resources_tests
branch
from
afcbf1e to
3e78f39
Compare
…80905/offload-test-suite into add_unmapped_resources_tests
3 participants
This PR is the first step in bringing support to testing with partially mapped and unmapped resources.
It lays out the infrastructure necessary to initialize resources with only a certain number of tiles mapped.
The Load overload that takes the 2nd status argument is implemented in DXC, so we take advantage of that and test that when accessing data in an unmapped resource, we get 0'd data and the status integer is also 0.
The test is currently unsupported in Clang and Vulkan, but will be supported in the future.
Fixes #182