Add capability to use reserved or committed resources for SRVs and UAVs #604
Conversation
…b.com/bob80905/offload-test-suite into add_unmapped_resources_tests_for_UAVs
bob80905
changed the title
damyanp
left a comment
damyanp
left a comment
There was a problem hiding this comment.
I think there may be some unrelated changes in this PR that should be pulled out into a separate, and also some changes that probably aren't really necessary.
lib/API/DX/Device.cpp
Outdated
| CD3DX12_HEAP_PROPERTIES(D3D12_HEAP_TYPE_UPLOAD); | ||
| const D3D12_RESOURCE_DESC UploadResDesc = | ||
| CD3DX12_RESOURCE_DESC::Buffer(BufferSize); | ||
| const D3D12_HEAP_PROPERTIES UploadHeapProps = |
There was a problem hiding this comment.
I suspect that swapping the order of UploadResDesc and UploadHeapProps wasn't particularly intentional? Might be good to swap them back to reduce churn in the PR.
There was a problem hiding this comment.
Renaming UploadHeapProp -> UploadHeapProp - I can see why it makes sense, but I'm not sure it really belongs as part of this change. If we are going to change the naming convention then we should also rename ReadBackHeapProp, but then we're churning things even more.
lib/API/DX/Device.cpp
Outdated
| return Err; | ||
| } | ||
|
|
||
| // Committed upload buffer |
There was a problem hiding this comment.
I don't quite understand the rationale for adding these "Committed XXX buffer" comments.
These will always be committed resources, won't they? And they're used for uploading and reading back data to/from the resource whether it is committed or reserved.
There was a problem hiding this comment.
Yeah. I thought it was helpful to write them for me personally. They indeed will always be committed resources, which was counter intuitive to me as I was writing the reserved resources changes, as I assumed everything should be using the CreateReservedResource API.
I can remove them if they're unnecessary, but that was my rationale.
damyanp
left a comment
damyanp
left a comment
There was a problem hiding this comment.
LGTM
Couple of comments left open, but they're take-it-or-leave-it.
lib/API/DX/Device.cpp
Outdated
|
|
||
| ID3D12CommandQueue *CommandQueue = IS.Queue.Get(); | ||
| CommandQueue->UpdateTileMappings( | ||
| Buffer.Get(), 1, &StartCoord, &RegionSize, // One region |
There was a problem hiding this comment.
This comment is confusing. Is it referring to one of the 1 arguments? Is it repeating the comment from line 580 above? Having a comment in the middle of an argument like this is generally avoided in any case.
| Buffer.Get(), 1, &StartCoord, &RegionSize, // One region | |
| Buffer.Get(), 1, &StartCoord, &RegionSize, |
…Vs (llvm#604) This PR is the second step in bringing support to testing with partially mapped and unmapped resources. It lays out the infrastructure necessary to initialize UAV resources with only a certain number of tiles mapped. By default, if the UAV is specified as reserved, a sufficient number of tiles to cover the entire size of the UAV resource will be mapped, unless otherwise specified. The `IsReserved` keyword was added to the yaml resource setup, and if not specified, the resources will be created as committed. The TilesMapped keyword requires that the IsReserved keyword is set to True. Addresses llvm#182 Fixes llvm#181 Fixes llvm#623 The last part needed to complete the above issue (182) would be to implement partial mappings for Buffers, but that will be left to a separate PR.
3 participants
This PR is the second step in bringing support to testing with partially mapped and unmapped resources.
It lays out the infrastructure necessary to initialize UAV resources with only a certain number of tiles mapped. By default, if the UAV is specified as reserved, a sufficient number of tiles to cover the entire size of the UAV resource will be mapped, unless otherwise specified.
The
IsReservedkeyword was added to the yaml resource setup, and if not specified, the resources will be created as committed. The TilesMapped keyword requires that the IsReserved keyword is set to True.Addresses #182
Fixes #181
Fixes #623
The last part needed to complete the above issue (182) would be to implement partial mappings for Buffers, but that will be left to a separate PR.