Repository navigation
feat(agc): implement sint storage images through uint views - #944
Conversation
adriadam10
left a comment
There was a problem hiding this comment.
Measured on the HW oracle (gfx1035, raw image_store + read-back): 8 formats × dmask 0xf/0x5 × 24 edge values. With X,Y,0,1 selectors on the two-channel formats all 448 rows match this PR (per-component saturation, zero for components outside dmask, raw 32-bit). Build + ctest green, and the new test fails without the change. Three things before merging:
ImageStoreSint.cpp:34/:289: the test stores to 8_8, 16_16 and 32_32 SINT with DST_SEL X,Y,Z,W (0xfac). With that T#, the hardware writes channel 1 of a two-channel format from the W data slot, not from Y. dmask 0xf of (0x11, 0x22, 0x33, 0x44) stores (0x11, 0x44), and dmask 0x3 stores (0x11, 0). So the dmask 0xf rows pin values the console doesn't produce (86 rows differ on 8_8/16_16/32_32).StoreTexelbehaves the same for UINT onmain, so it predates this PR. Please use X,Y,0,1 (0x22c) for the two-channel formats so the test only pins what the hardware does. The W-slot aliasing can go to TechnicalDebt or a separate PR.TechnicalDebt.md:146: image stores are measured now. Either drop the line or say that they were measured on RDNA2 (gfx1035).ShaderResources.cpp:1060conflicts withmainafter #896 (Atomic64View). Please rebase and keep both branches, as you noted in the description.
Storage images of a SINT format threw "storage image descriptor uses an unsupported format"; only write-only 32_SINT was accepted. The recompiler now writes 16_SINT, 8_8_SINT, 32_SINT, 16_16_SINT, 8_8_8_8_SINT, 32_32_SINT, 16_16_16_16_SINT and 32_32_32_32_SINT storage images with the Uint numeric class. 8- and 16-bit components are saturated (EmitFormatStoreComponent) and 32-bit ones stored unchanged, as measured on RDNA2 (gfx1035); image_store_pck stores 32-bit components unchanged too. The driver binds these surfaces through a cached view of the same-size UINT format. 8_SINT, 32_32_32_SINT, the packed SINT formats (11_11_10, 10_11_11, 2_10_10_10, 10_10_10_2), D16 stores, packed stores of 8- and 16-bit components and atomics other than 32-bit ones on 32_SINT and 64-bit ones on 32_32_SINT still throw. New test: agc_driver_image_store_sint, with DST_SEL X,Y,0,1 on the two-channel formats, as with X,Y,Z,W the hardware writes channel 1 from the W data slot (TechnicalDebt).
4bad445 to
6e34e86
Compare
|
Thanks for measuring the stores on gfx1035. The 448 matching rows are exactly what the PR was missing, and I hadn't noticed that with X,Y,Z,W the hardware writes channel 1 from the W slot. I've force-pushed 6e34e86 with all three points:
|
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. Checked 6e34e86 on RADV (gfx1035 oracle for the hardware side):
- Oracle: the 448
image_storerows (8 formats × dmask 0xf/0x5 × 24 edge values, X,Y,0,1 on two-channel formats) and the test's other forms (dmask 0x1 glc with X,1,1,1 on 16_SINT,image_store_pckdmask 0x3 on 32/32_32/32_32_32_32_SINT, 128 rows) all match. - Build + ctest green (393/393), and
agc_driver_image_store_sintfails without the change. - TechnicalDebt lines and the rebase onto #896 look right.
Merging.
What
SINT storage images throw
storage image descriptor uses an unsupported format, except write-only 32_SINT.EmitFormatStoreComponent); 32-bit SINT formats store raw dwords, also withimage_store_pck.StoreTexeltakes Y, as it does for UINT onmain(TechnicalDebt).StorageTexture::StorageViewbinds a cached same-size UINT view; loads keep the sign-extending SINT view.main),image_store_pckof 8- and 16-bit components, and atomics other than 32-bit ones on 32_SINT and 64-bit ones on 32_32_SINT.StoreTexeland the storage descriptor write; whichever lands second keeps the SINT saturation andStorageView. feat(agc): implement image and sampler tables indexed at runtime offsets #757-feat(agc): implement deferred faults for reads through null SRT pointers #759, feat(agc): implement image_load through image tables #920, feat(agc): implement 8 and 8_8 srgb image loads through unorm views #866, feat(agc): implement 8 srgb color targets through rgba8 srgb proxies #1058 and feat(agc): host import windows, least-recent eviction under the budget #290 edit the same functions and merge cleanly with this.Tested
main878e6a6: configure-DBUILD_TESTING=ON, full build,--target libs,ctest:ctest --output-on-failure)VK_DRIVER_FILES): 387/387 pass;agc_driver_flat_storeis left out there because it takes several minutes on Windows llvmpipeagc_driver_image_store_sint: eight formats, dmask 0xf and 0x5, 24 edge values and X,Y,0,1 on two-channel formats, animage_loadread-back,image_store_pckon the 32-bit formats, the title's store (dmask 0x1 glc, DST_SEL X,1,1,1) and six throws; fails onmainwith the message above.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository (attach them to this PR)