Repository navigation
fix(shader): count folded draw offsets once in the vertex and instance indices - #2285
Conversation
|
One remaining semantics question from the full diff: |
b8a86aa to
6fb6049
Compare
…e indices When the embedded vertex fetch adds the base vertex or start instance user SGPR to the index VGPR, the driver folds that SGPR into the draw's first vertex and first instance. v5/v8 were gl_VertexIndex and gl_InstanceIndex, which already include it, so the translated add counted it twice, on direct draws and CPU-path indirect records alike. The entry of a vertex or local program now subtracts the folded SGPR's user data word from the builtin, which gives the zero-based indices the hardware has. A record drawn in place on the GPU keeps the CPU's word, so the fetch analyzer counts the SGPR as shared unless the fetch add is the only reader of v5/v8 and the SGPR (no earlier access or EXEC write, no carry, clamp, second add or other write of the SGPR); such records take the CPU path. The fold moves into Driver::FoldDrawOffsets, ClassifyIndirectDraw becomes public for the test, and the env switches that skipped the fold go.
6fb6049 to
fcc5105
Compare
|
You're right that the in-place case only proved the value after the add, so I went back to what the hardware puts in these registers. Neither the old pre-add values nor my first version's were the hardware's: on GFX10 the VertexID/InstanceID VGPRs are zero-based, and the base vertex and start instance reach the shader only through the user SGPRs the draw names, which the CP fills from the record for DRAW_INDIRECT and DRAW_INDEX_INDIRECT (radeonsi and RADV both rely on this, sources below). For the test's in-place record (vertex offset 700, start instance 59,455, stale user data 0x51/0x62) the hardware has v5/v8 = index/instance before the adds; main gives index + 700 / instance + 59,455 there, since gl_VertexIndex/gl_InstanceIndex already contain the record, and my first version gave index + 619 / instance + 59,357. So I made everything a program can observe match the hardware instead. In an in-place draw the shader's s16 still holds the CPU's word rather than the record's, so the analyzer now keeps a record in place only when the fetch add is the only thing that sees v5/v8 and the SGPR. A program that touches v5/v8 or writes EXEC before the add, adds the SGPR into another VGPR, reads the add's carry, clamps it, adds the SGPR twice, writes the SGPR through a carry or lane mask, or addresses SGPRs relatively counts the register as shared, and its records take the CPU path, where both values are exact. Programs that pass have no pre-add value anything can read, and their post-add value is gl_VertexIndex itself, so the TechnicalDebt entry is gone. There's no request or cache layout change, and nothing moves in Yotei: all 28 of its programs that fold an SGPR also read or rewrite it elsewhere, so their records already took the CPU path. The test now checks both values on every path that has a pre-add value, including the in-place-shaped record whose program copies v5/v8 ( DetailsSources (Mesa main ed458ffa):
Values per path (U = the CPU's word in s16, R = the record's offset, i = index; instance analogous; before / after the add):
(i includes GE_INDX_OFFSET wherever the driver already applied it.) Why not gl_BaseVertex/gl_BaseInstance for the SGPR: that makes the in-place value exact too, but on draws with a GE_INDX_OFFSET firstVertex also carries that offset, so BaseVertex isn't the SGPR's value there. It needs a per-draw value or a shader variant per path, plus Test cases (NVIDIA RTX 3080 Laptop and Windows llvmpipe): eight programs check the analyzer (copies v5/v8 first, adds only, reads the instance add's carry, clamped v_sad_u32, s16 added twice, EXEC written first, s17 overwritten by another add's carry, s16 added into v0); direct auto and indexed draws; an indexed and an auto record of the copying program, classified to the CPU path and drawn with both values checked; an indexed record of the adds-only program, classified in place and checked after the add; and an indexed record whose packet names no register, drawn on the GPU with both values checked. With main's translator the test fails as before (start instance 40 gives v8 = 40 and 80); with the analyzer change reverted, the copying program's records are classified to the GPU. The full suite on fcc5105 passes: 508/508 on Windows, 517/517 in the Linux job merged with main d70b899. |
|
Checked on Linux (build + ctest): Heads-up: this has adjacency conflicts with #2313 ( |
…ouble-add fix(shader): count folded draw offsets once in the vertex and instance indices
What
An embedded vertex fetch adds the base vertex or start instance user SGPR to the index VGPR (
v_add_co_u32 v8, vcc, s19, v8), and the driver folds that SGPR into the Vulkan draw's first vertex and first instance so the replaced attribute fetch starts there. v5 and v8 weregl_VertexIndexandgl_InstanceIndex, which already hold that offset, so the translated add counted it twice: index + offset before the add, index + 2 × offset after it. This hits direct draws and indirect records drawn on the CPU path alike; PPSA26344 indexes a per-instance buffer with v8 + s19 at start instances up to 59,455.Driver::FoldDrawOffsetswith the same rules.APS5_INDX_OFFSET_SKIP_FOLDandAPS5_NO_INDEXED_OFFSET_FOLDgo, as skipping the fold would now leave the indices short.si_emit_draw_packets, RADVradv_cs_emit_indirect_draw_packet).EmbeddedVertexFetchAnalyzernow counts the register as shared unless the fetch add is the only reader of the index VGPR and the SGPR (no earlier read, carry, clamp or second add); such records take the CPU path, where both values are exact.foldlambda, which keeps its signature.Tested
maind70b899: configure-DBUILD_TESTING=ON, full build,--target libs,ctest:ctest --output-on-failure)maind70b899: 517/517 passagc_driver_draw_offset_indices: a vertex program stores v5/v8 before and after its fetch adds for direct draws, records the now publicClassifyIndirectDrawsends to the CPU path, one drawn in place and one whose packet names no register; it also checks which programs may stay in place. With main's translator it fails (at start instance 40, v8 reads 40, not 0, and 80 after the add; the 59,455 record gives 118,910), and without the analyzer change a record whose program copies v5/v8 first stays in place.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)