Repository navigation
fix(agc): color compare emulation: review follow-up to #668 - #907
ThisIsAkill wants to merge 3 commits into
Conversation
26c91e6 to
3200255
Compare
|
Removed code comments per CONVENTIONS.md; no functional change. |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. I measured the four points on RDNA2 (gfx1035, image_sample_c_lz on a 32_FLOAT texture, point sampling) with the HW oracle (tools/hw-oracle/):
- Array layer rounding (blocking). The hardware selects the layer as
clamp(roundEven(z), 0, layers - 1), notfloor(z + 0.5): with 4 layers, z = 0.5 → 0, 1.5 → 2, 2.5 → 2, 3.5 → 3, 0.49999997 → 0 (in f32,0.49999997 + 0.5rounds to 1.0, sofloorpicks layer 1 there too).SpirvImageEmitter.cpp:936(from #668) usesFloor(z + 0.5), and the new test pins z = 0.5 → layer 1 (EmulatedColorCompare.cpp:129,Expectedat :168), i.e. it locks in the value the hardware disagrees with.GLSLstd450RoundEven(as Vulkan does for its own array layer) and expected values from round-half-even, with rows 0.5, 1.5, 2.5 and 0.49999997 over at least 3 layers, would fix both. - NOT_EQUAL. Measured: NOT_EQUAL returns 1 when the reference, the texel or both are NaN; the other functions return 0 for NaN; +0 and -0 compare equal. So
OpFUnordNotEqualmatches. The TechnicalDebt line can say it was measured on RDNA2 instead of "not verified". No test row covers it now (the test never runs NOT_EQUAL, so it passes withOpFOrdNotEqualtoo): a case on an emulated float format (e.g. 16_16_16_16_FLOAT; R32 float takes the native path) with a NaN texel and a NaN reference under NOT_EQUAL would pin it. - Rebase. This conflicts with
main: #815 already throws for a min/max reduction sampler inemulatedCompareStateand adds the matchingRejectin this test. Please drop the reduction check, itsRejectand thereductionfield, and keep theTRUNC_COORDpart. - TRUNC_COORD with bilinear is accepted. On hardware it changes the bilinear weights (e.g. 2.12109 instead of 2.125 at u = 2.4999 texels). That is the same gap as the existing "full float weights" sentence, so either throw for it too or name it in that sentence.
…nt sampling in the color compare emulation NOT_EQUAL is unordered (true when the reference, the texel or both are NaN) and the other functions are false for NaN, as adriadam10 measured on RDNA2 in the boykopovar#907 review; OpFUnordNotEqual matches. TRUNC_COORD with point sampling changes which texel is read, which the emulation does not model, so the specialization throws for it like the other out-of-scope sampler state; with bilinear filtering it changes the weights, the same gap as the full-float weights, and is documented with them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ation A four-layer array view with layer coordinates at the rounding points (0.49999997, 0.5, 1.5, 2.5, 3.5) and past the clamp, and neighbouring layers far apart in value, so floor(z + 0.5) and round-half-even give different results; point, bilinear, wrap and border sampling. GREATER next to LESSEQUAL. NOT_EQUAL and LESSEQUAL on a 16_16_16_16_FLOAT texture with NaN and signed-zero references and texels. The TRUNC_COORD rejection. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The emulated comparison picked the array layer as floor(z + 0.5). Measured on RDNA2 (adriadam10, in the boykopovar#907 review), the hardware takes clamp(roundEven(z), 0, layers - 1): z = 0.5 selects layer 0 and 2.5 layer 2, and 0.49999997, which floor(z + 0.5) rounds up to 1 in f32, selects 0. GLSL.std.450 RoundEven matches that and is the rule Vulkan uses for its own array layer. The floor(z + 0.5) came in with boykopovar#668. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Note: the branch is rebased onto current @adriadam10 thanks for measuring this on hardware. All four points are addressed, and the branch is rebased onto current
Tests: |
0e9e4b5 to
593ffa1
Compare
|
Ready to merge from my side, but the PR description fails the conventions check:
Please fill those template lines in (only you can answer AI-assisted); it merges once they pass. |
|
Conflicts with main in |
…t) (#481) * fix(recompiler): give vertex-path shaders the NGG group info Ported from upstream PR boykopovar#5081 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): put the compute wave index in TG_SIZE[11:6] Ported from upstream PR boykopovar#5214 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): take the 32_AR export alpha from channel 1 as GFX10 does Ported from upstream PR boykopovar#5110 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): order buffer and image memory at s_barrier Ported from upstream PR boykopovar#5117 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): decode GFX10 scratch offsets as one dword Ported from upstream PR boykopovar#5426 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): accept MIMG sampler tuples ending in VCC Ported from upstream PR boykopovar#4996 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(recompiler): name the attribute of an out-of-range interpolation reference Ported from upstream PR boykopovar#5450 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): decode the high-half result of v_interp_p2_f16 Ported from upstream PR boykopovar#5471. The Vop3InterpDecode unit-test hunk is not ported (that test file is not on main); the execution test InterpolationF16 carries the coverage. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): clear the bits of inactive lanes in VALU carry-out and v_div_scale masks Ported from upstream PR boykopovar#5162 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): apply the f32 output modifier and denormal flush to v_cvt_f32_f64 Ported from upstream PR boykopovar#5159 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): update per-invocation LDS without atomics Ported from upstream PR boykopovar#5160. Conflicts resolved against main: kept main's barrier condition for non-LDS shared atomics; the unit-test hunk for SRT read patching (a different upstream change, not on main) is dropped. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): flush ds_add_f32 denormals and NaNs as the hardware does Ported from upstream PR boykopovar#5492 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(shader): run DS src2 lanes one at a time in lane order Ported from upstream PR boykopovar#5498 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(agc): compare NOT_EQUAL unordered, reject TRUNC_COORD point sampling and round the array layer half to even in color compare emulation Ported from upstream PR boykopovar#907 (follow-up to its predecessor). Hand-applied to main's reworked emitter: NOT_EQUAL uses OpFUnordNotEqual, the array layer is clamp(roundEven(z)), and ResourceMaterializer rejects TRUNC_COORD with point filtering. The upstream execution-test rewrite is not ported (it targets a diverged test file). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * chore(shader): bump ShaderDiskCache FormatVersion to 35 The ported DS fixes add MemoryInfo::flushDenormalResults (now encoded in the plan) and new IR opcodes, so cached payloads from format 34 must not load. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): run ported upstream execution tests on Metal through a device shim The upstream execution tests for the ported fixes compile unchanged against a Metal stand-in for the Vulkan test device (guest buffer descriptors are mirrored into shared MTLBuffers). Covers boykopovar#5162 (VALU carry-out masks), boykopovar#5159 (v_cvt_f32_f64 result modes), boykopovar#5492 (ds_add_f32) and boykopovar#5498 (DS src2 lane order). The LdsSrc2 and LdsSrc2Wide tests use device-memory LDS, which the Metal bridge rejects, and are not registered. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): run ported decoder and collector unit tests in the Metal build Registers the upstream tests for boykopovar#4996, boykopovar#5426 and boykopovar#5450 (already on main's tree but not built for Metal) and a high-half decode check for boykopovar#5471. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): check per-invocation LDS updates without atomics on the Metal recompiler Standalone copy of the boykopovar#5160 verifyFunctionLdsUpdates check. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): check the TG_SIZE wave index on Metal Buffer-store variant of the boykopovar#5214 execution test. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): run the s_barrier memory-order execution test on Metal Registers boykopovar#5117's upstream test through the device shim. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): draw a 32_AR export on Metal Guest-replay draw for boykopovar#5110: alpha comes from export channel 1. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): draw an NGG vertex shader that reads the group info Guest-replay draw for boykopovar#5081. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): reject TRUNC_COORD point sampling in the color compare emulation Recompile-level check for boykopovar#907. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(shader): run the src2 lane-order test without the Metal debug layer Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * docs(shader): port upstream debt entries and add the FormatVersion 35 history line Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
Update after review: rebased onto
maincea1ef13. The reduction check is dropped (#815 has it). The array layer is now round-half-even (measured on RDNA2 by adriadam10), correctingfloor(z + 0.5)from #668. NaN rows were added, and TRUNC_COORD with bilinear is documented. See the reply below for details.What
A follow-up to #668, which was merged before the review fixes were pushed. It carries the changes that answer the four review points from that PR; nothing else. Two commits on current
main:fix(agc): compare NOT_EQUAL unordered and refuse reduction filters and TRUNC_COORD in the color compare emulationtest(agc): cover 2D arrays and GREATER in the color compare emulationHow each review point is addressed
EmulatedColorComparegains a 2D array case (two layers, a different red table per layer): it covers the layer size query and thefloor(z + 0.5)clamp, with layer coordinates below 0, between layers and past the last one.NOT_EQUALis nowOpFUnordNotEqual(true when the reference or the texel is NaN). I could not verify the hardware's NaN behaviour, so the TechnicalDebt entry says so: it is the IEEE!=and as Vulkan defines it for its own depth compare, not confirmed on hardware.FILTER_MODE(word 0 bits 29 to 30) with a min/max reduction mode throws, and so doesTRUNC_COORD(bit 27) with point sampling. The test checks both are rejected and that the default values still run. The TechnicalDebt entry lists both.GREATERas well asLESSEQUAL, which guards the operand order.Notes
main) touchesResourceMaterializer.cpp, which this change edits by four lines. Whichever lands second rebases; the emulated compare then needs the runtime-mode form you described in the review.Tested
Linux, GCC, NVIDIA RTX 3070, on
mainat5895ea72:agc_driver_emulated_color_comparepasses, 4 runs in a row (the new 2D array, GREATER and rejection cases included).ctestpasses: 359 of 359, with one test (agc_driver_pixel_valid_mask) skipping itself.I did not run the new cases against the unfixed code in this round.
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)