Repository navigation
fix(agc): bind guest buffer views that start off a DWORD boundary - #1270
Conversation
|
Linux results, since this was tested on Windows only. Nobara 44 (Linux 7.2), GCC 16.2.1, Release,
|
|
Thanks, that's the Linux and RADV coverage this one was missing. Good to know the shader_clock failure is #985 and not this. |
adriadam10
left a comment
There was a problem hiding this comment.
Measured on RDNA2 (gfx1035, HW oracle) with V# bases 0-3 bytes off a dword boundary:
- Raw
buffer_load_dword/dwordx2(with and without glc),ushortandubyteread the exact bytes at base + offset. The range check works per dword: a dword that is partly pastnum_recordsreturns 0. Raw stores (dword,short,byte,dwordx2,tbuffer8_UINT) write the exact bytes and drop a dword that is partly out of range.s_buffer_load_dwordreads(base & ~3) + (offset & ~3). All of this matches what the PR emits and whatBufferUnalignedBaseexpects. - Formatted accesses fault when the address isn't aligned to the element.
tbuffer_load_format_x16_UINTat an odd address,32_UINTor16_16_UINTat an address that isn't a multiple of 4, andtbuffer_store_format_x16_UINTat an odd address all fault the wave (the kernel hangs). The same instructions work once the address is aligned (16_UINTbase +1 offset 1,32_UINTbase +1 offset 3,8_UINTat any address). With views +1 and +3,tbuffer_load_format_x 16_UINT ... offset:2lands on odd addresses, so the hardware faults there. The PR returns data instead, andBufferUnalignedBase.cpp:108pins that data. Under "exact or throw", formatted loads and stores through a V# whosebaseMisalignmentisn't a multiple of the element's alignment (min(element bytes, 4)) should throw at recompile time, and the test should check that they throw. The8_UINTstore row can stay. docs/dev/TechnicalDebt.md:305: the raw behaviour is now measured, so the line can say so (gfx1035 on Linux, whose alignment mode is UNALIGNED; the console's mode is still unknown). Keep the part about vector offsets that aren't a multiple of 4: the hardware reads the exact bytes there, while the recompiler counts dwords from the V# base. That comes frommain, not from this PR.
Build, ctest (435/435) and the negative run (both agc_driver_graphics and agc_driver_buffer_unaligned_base fail without the change) pass here. The conflicts with #1364, #1366 and #1202 are the usual ones between open PRs: whichever lands second rebases.
A V# whose base is not a multiple of 4 bytes from the start of its GPU owner threw in GuestBufferMemory::Descriptor, because the shader added the offset below the view only in whole dwords. Descriptor now binds from the offset alignment below the view and passes the whole remainder. Such a view's range covers the dwords around it, and its descriptor region starts and ends on dword boundaries, so a lone view copied to its own buffer binds the same way. The buffer specialization records the V# base's low two bits. Raw vector loads through such a V# join two dwords, glc dwordx2 loads included, stores merge their bytes into both with compare-exchange loops, and bounds checks include the second dword. s_buffer_load ignores the low two bits, as SrtEvaluator does. Formatted accesses whose element alignment (min(element bytes, 4)) the base breaks throw, as they fault the wave on RDNA2, and so do atomics.
246e0c8 to
38e89df
Compare
|
Thanks for running these on real hardware. The formatted-access faults were the one part I had no way to check, and you're right that the old test pinned data the GPU never returns. I've pushed 38e89df. Formatted loads and stores through a V# whose base misalignment isn't a multiple of the element's alignment, min(element bytes, 4), now throw at recompile time. I also checked what Yotei does with these views. Its 146 programs that bind one only read them with formatted 16_FLOAT loads at base +2 and 8-bit formats, so none of them reaches the new throw. Could you take another look when you have time? Details
|
adriadam10
left a comment
There was a problem hiding this comment.
Checked the formatted-access throw added in 38e89df against the earlier gfx1035 measurements: load and store paths both reject a V# base that breaks the element's alignment, the test pins the throws plus the 16_UINT +2 and 8-bit passes, and TechnicalDebt carries the measured raw behaviour. Build and ctest (451/451) pass, and agc_driver_graphics / agc_driver_buffer_unaligned_base fail without the change. Merging once CI is green.
What
A V# whose base is not 4-byte aligned within its GPU owner can't be bound:
GuestBufferMemory::Descriptorthrowsguest buffer view off the storage buffer offset alignment is not DWORD aligned, as the shader adds the offset below the view only in whole dwords.Descriptorbinds from the offset alignment below the view (at least 4), passes the whole remainder and ranges over the dwords around it (ViewBytes); lone copies bind the same way. An owner off a DWORD boundary throws, as does a view whose dwords cross its owner's end.dwordx2too), stores merge into both by compare-exchange, bounds checks include the second dword ands_buffer_loaddrops the two bits, as RDNA2 (gfx1035) does.DRAW_INDEX_2per present into the G-buffer with this error; its 146 programs with such views only make 16_FLOAT (base +2) and 8-bit formatted loads through them.GuestBufferMemory. perf(agc): pipelined draws on by default #1202 adds a copy path next to the snapshot size this rounds withViewBytes; the merge should keep the rounded size.Tested
mainc5e1018: configure-DBUILD_TESTING=ON, full build,--target libs,ctest:ctest --output-on-failure)VK_DRIVER_FILES): 395/395 pass;agc_driver_flat_storeis left out there because it takes several minutes on Windows llvmpipeagc_driver_buffer_unaligned_base: raw and 8-bit formatted loads and stores, scalar loads, and 16_UINT formatted ones at even addresses through views 1-3 bytes off; 16_UINT accesses at odd addresses and 32_UINT and 16_16_UINT loads off 4 bytes throw.agc_driver_graphics: the offsets and ranges. Both fail onmainwith that error.not DWORD alignedskips, where a 900 s run without this change skipped 17DRAW_INDEX_2per present (4,314).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)