Repository navigation
fix(shader): translate v_mullit_f32 in f32 denormal modes 1 and 2 - #5941
Conversation
jotitadev
left a comment
There was a problem hiding this comment.
Run on an Apple GPU, since the PR was tested only on lavapipe. agc_driver_mullit_f32_denormal_modes fails here, because the test needs f32 denormals to be preserved and this device does not preserve them.
GPUs tested: Apple M2 Max, MoltenVK 1.4.2 (Vulkan 1.4.357), macOS 27.2, x86-64 under Rosetta; also MoltenVK main built with KhronosGroup/SPIRV-Cross#2704 (keeps the sign of -0)
main does not run the GPU tests on macOS yet, so I tested on macos-port at b7eb6b6b (mugurc/AnyPS5) plus the ImportWatched change from #1, with this PR's own diff (merge-base..head 5279465e1) applied. Not main. Release build, LunarG Vulkan SDK 1.4.363.0. Suite: ctest -R "^agc" -E "agc_display_buffer_dcc|agc_driver_flip|agc_driver_lds_atomics64" -j1 --timeout 30. Without the PR, 13 tests fail on this setup (7 with the #2704 build), all known macOS failures listed in #1. The diff applies cleanly.
- With only the new test added to the unchanged branch, it fails with
f32 denormal mode 1 at pc 24 is not implemented. So the test runs on this device, and the translation now accepts modes 1 and 2. - With the PR, it fails with both builds:
stock: v_mullit_f32 denorm32 1 no ieee inputs 0x00000001 0x80000001 0x00800000 is 0x00000000, expected 0x80000000 #2704: v_mullit_f32 denorm32 1 no ieee inputs 0x00000001 0x7f7fffff 0x00800000 is 0x00000000, expected 0x34ffffff - To see every failing row, I changed the
RequireinChecklocally to print each mismatch. The test has 23 rows × 8 modes.- Stock MoltenVK: 34 mismatches. 12 of them are -0 returned as +0 (the SPIRV-Cross issue in #1), and they go away with the #2704 build.
- #2704 build: 22 mismatches. Every one returns a zero where the expected value depends on keeping an f32 denormal:
- Mode 0 passes every row.
- Mode 1 fails 1 row, at both IEEE settings:
0x00000001 * 0x7f7fffff, where the denormal source is flushed before the multiply. - Mode 2 fails 1 row, at both IEEE settings:
0x00800000 * 0.5, where the0x00400000result is flushed. - Mode 3 fails 9 rows, at both IEEE settings: denormal sources or results, for example
0x00000001 * 1.0gives0, not0x00000001.
- This device reports
shaderDenormPreserveFloat32 = false(andshaderDenormFlushToZeroFloat32 = false).agc_driver_f32_denormal_flushalready fails on this branch without the PR. The mode 3 rows exercise translation that is already onmain. - Suite: the base failures plus
agc_driver_mullit_f32_denormal_modes, with both builds. No other test changes.
So the new modes 1 and 2 look right on Apple for every row that does not need a preserved f32 denormal. The test fails here only because of a device capability. If these tests are to run on hosts without f32 denormal preservation, the rows that need it could be skipped when that feature is missing, as #5492 does for ds_add_f32.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
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.
|
adriadam10
left a comment
There was a problem hiding this comment.
I measured v_mullit_f32 on RDNA2 (gfx1035) with the hardware oracle: 13,824 operand sets in denorm32 modes 0..3 at IEEE 0 and 1, round to nearest even. The register-only form matches your gfx1030 numbers, and with this PR the recompiler matches every row on CI's lavapipe. Two problems are left:
-
Mode 1 with a constant factor skips the result flush, on every driver.
flushTinyProduct(Translation/src/TranslationContext/OperandAccess.cpp:159, called fromFloatInstructions.cpp:443) returns the plain product when a factor is a constant with exponent 0 or ≥ 127 (keepsProductOutOfTinyRange). That shortcut is only valid when the sources are flushed. In mode 1 they are kept, so a tiny product goes through. Oracle vs this PR, on both lavapipe and RADV, with src2 = 1.0:v_mullit_f32 v, 1.0, v5, v6with v5 =0x00000001: hardware0x00000000, PR0x00000001v_mullit_f32 v, v4, 2.0, v6with v4 =0x00000001: hardware0x00000000, PR0x00000002v_mullit_f32 v, 0x1 (literal), v5, v6with v5 = 1.0: hardware0x00000000, PR0x00000001
Over the sweep that is 1,560 rows each for the inline 1.0 forms, 624 for inline 2.0 and 936 for the literal. Applying the shortcut only when
f32DenormalFlush & 1uis set fixes it. Please also add test rows with an inline constant operand. -
The new test fails on RADV (Mesa, gfx1035):
v_mullit_f32 denorm32 1 no ieee inputs 0x00000001 0x7f7fffff 0x00800000 is 0x00000000, expected 0x34ffffff. jotitadev hit the same row on MoltenVK. Nothing asks the driver to keep f32 denormals: onlyDenormPreserve 16and64are ever requested (VulkanDevice.cpp:1284,:1495,SpirvModuleSetup.cpp:337). RADV flushes f32 denormals by default, and lavapipe keeps them. So on RADV, mode 1 gives 0 for0x00000001 * 0x7f7fffffand mode 2 gives 0 for0x00800000 * 0.5(hardware0x00400000). Before this PR those modes threw. One possible fix:- Request
DenormPreserve 32(gated onshaderDenormPreserveFloat32, like the f16/f64 flags) for shaders whose f32 denormal field keeps sources or results. - Keep throwing for modes 1 and 2 on devices without that feature, and skip those rows in the test there.
The mode 3 rows have the same gap on
maintoday. Either cover them the same way, or leave them out of this test so it does not fail on drivers that flush. - Request
5279465 to
eae67fc
Compare
|
Thanks for measuring on gfx1035 and for both findings. Updated in eae67fc, rebased onto main cecb540. Constant factors. The keepsProductOutOfTinyRange shortcut in flushTinyProduct now applies only when the sources are flushed (f32DenormalFlush & 1u), as you suggested. The same shortcut also hit v_mul_f32 and v_mul_legacy_f32 in mode 1 (#6010), so the fix lives in #6010's commit. With #6010's result-only flush fix, a denormal result in mode 1 is flushed as well. Values, with preserve forced in the test and the harness: the test checks all 16 runs, and all 110,592 register rows (13,824 × 8 modes) and all 1,632 × 5 constant-form rows per mode match. |
…nd 2 v_add_f32, v_sub_f32, v_subrev_f32, v_mul_f32 and v_mul_legacy_f32 follow the FLOAT_MODE f32 denormal field with f32 round to nearest even, as measured on RDNA2 (gfx1030): mode 1 keeps denormal inputs and flushes a denormal result, and mode 2 flushes denormal inputs and keeps a denormal result. Both need the device to keep the f32 denormals the shader keeps, so they translate only when the module declares DenormPreserve 32 and throw otherwise. The result flush now applies when only the result flush bit is set; it was skipped unless the input flush bit was set too. A constant factor no longer skips the tiny product check while denormal inputs are kept, as a kept denormal times such a constant can still be tiny. Directed rounding in these modes still throws. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Denormal mode 2 flushes only the sources and mode 1 only the result, as measured on RDNA2 (gfx1030), also with an inline or literal constant factor. Both need the device to keep the f32 denormals the shader keeps, so they translate only when the module declares DenormPreserve 32 and throw otherwise; mode 1 with another f32 rounding throws as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
eae67fc to
c33e289
Compare
|
Re-checked against current |
What
A shader whose
FLOAT_MODEf32 denormal field is 1 or 2 failed to translate onv_mullit_f32("f32 denormal mode N is not implemented"). Measured on hardware, the two modes do what their names say:0x00000001 * 1.0gives+0.f32DenormalFlushFornow returns these flags forv_mullit_f32: sources only for mode 2, result only for mode 1. The existing input flush and tiny-product check then produce the measured results. Mode 1 is only accepted with f32 round to nearest even, the rounding that was measured; with another f32 rounding it still throws. Every other instruction still throws in modes 1 and 2.Source: measured on RDNA2, gfx1030 (
RX 6800, Windows 11, AMD driver HIP runtime), with the hardware oracle's kernel template.Tested
Ubuntu 24.04, GCC 13.3, lavapipe, on
maina453108c.agc_driver_mullit_f32_denormal_modes(new) pins 23 measured rows, with an expected value for each of the 8 mode and IEEE combinations.mainit fails (mode 1 does not translate); with the change it passes in 0.3 s.agc_driver_f32_denormal_flushandagc_driver_f32_constant_factor_tininesspass.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt (n/a).mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code
https://claude.ai/code/session_01Ha6Tsy62AFUyp9a5Mf7vjA