Repository navigation
Conversation
|
Tested on AMD Ryzen 7 9800X3D and AMD Radeon RX 7900 XTX (Fedora Linux, Mesa 26.2.5):
It seems the 16-bit floating point rounding towards zero logic is prematurely truncating or zeroing out the store on RDNA3/RADV. Needs a revision! |
|
Thanks for running this on RADV. The |
|
I found the cause in the descriptor view path: the converted shader emitted UINT texels, but |
|
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.
|
|
Tested on AMD Ryzen 7 9800X3D and AMD Radeon RX 7900 XTX (Fedora Linux, Mesa 26.2.5):
|
|
The overlap report is valid: #8716 touches the |
|
Thanks for rerunning the corrected head. The focused pass on the RX 7900 XTX confirms the UINT storage-view fix for all three float16 formats; I updated the PR description with your exact-head result. I recorded the other test failures separately and am not attributing them to this change without their first failing assertions. |
|
An AMD proprietary-driver run, with the red control. Radeon 860M (RDNA 3.5, Edit: an earlier version of this comment read the row-1 failure below as a Windows write-back difference. That was wrong; the test's expectation for the masked component is what fails. As written, the test fails the same way on Row 1 stores with ACO relies on this to drop constant-zero components from the store's With that one line changed (
So on AMD's Windows driver the host's f32→f16 storage conversion already gives the round-toward-zero results for every edge value in the test, including 65520 → 0x7bff and the NaN payloads. The red control isn't red here, and the PR changes nothing on this driver. It is still needed wherever the host rounds to nearest. A red run on RADV or NVIDIA with the corrected expectation would show that. |
|
@MarcZX10 I corrected my earlier comment above. The row-1 failure isn't a Windows write-back difference: the test expects the component outside |
|
Fixed in follow-up commit 3764883: the execution test now expects zero for components omitted by the dmask, matching the behavior documented for this hardware generation and the existing ImageConvertedUnorm case. The focused target builds successfully on my Linux host. I attempted the CTest run, but this machine lacks LZCNT, BMI1, BMI2, MOVBE, AVX2 and FMA, so the test aborts before Vulkan initialization; no local GPU result is claimed. Hosted conventions, eligibility, labels and report checks are pending for the new head. |
|
RDNA 2 measurement behind Raw ISA on an RX 6700 XT (gfx1031) through the HIP runtime of AMD's Windows driver 32.0.21045.5002:
With the swizzles this test uses (0xfac for one and four channels, 0x22c for two):
This holds for A side observation that doesn't affect this test: under 0xfac, a two-channel format writes component 1 from data component 3 (W) and ignores data component 1. |
|
NVIDIA, Windows. GeForce RTX 3080, driver 616.92, Windows 11 Pro 26300, Ryzen 7 5800X3D, WinLibs GCC 15.2.0 r7, Release, Build. This PR doesn't build on current The merge is textually clean, so GitHub shows no conflict. For the runs below I passed Red control on NVIDIA. With this PR: the same test passes 3 of 3, with no validation messages. Regression. Under the layer it logs 8 From the code:
AI-assisted: yes |
|
Confirming @gdols's results on a second NVIDIA card, an RTX 4090 (Ada), at head
NVIDIA GeForce RTX 4090, driver 617.42, Windows 11 Pro 26200, WinLibs GCC 15.2.0 r7, Release, AI-assisted: yes (Claude Code) |
3764883 to
7081834
Compare
|
Thanks for the careful repro on both NVIDIA cards. I’ve pushed commit The updated head builds |
|
AMD Windows rerun at RX 6700 XT (gfx1031), AMD Windows driver 32.0.21045.5002, Windows 11 26200, MinGW GCC 15.2 Release,
Each cell gives the plain runs, then the validation runs:
At Each validation run logs 16 At |
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
The red control you were missing, from a round-to-nearest host: Apple M2 Max (Metal via MoltenVK).
Machine: Apple M2 Max (vendor=0x106b device=0x1b020208, subgroup 32), macOS 27.2, x86_64 under Rosetta 2. MoltenVK 1.4.2 (Vulkan SDK 1.4.363.0). Release, BUILD_TESTING=ON.
Base/method: PR #8658 feat/macos-port @ 4ebe71f2e5 (current main 0c6a5bd593 + the macOS port), not main. This PR (708183438) applied on top, clean, on its own (not stacked with other image PRs).
With this PR: agc_driver_image_store_float_rounding passes 3/3 (--repeat until-fail:3), image store float rounding tests passed. Because the write-only 16-bit-float store now converts to f16 in the shader and writes through the UINT view, the rounding is done in SPIR-V and is independent of the host's native image-store rounding.
Red control (round-to-nearest host): with only this PR's production files reverted to base and the new test kept — so the store goes back through the native SFLOAT view and Metal rounds the texel — the test fails:
AGC graphics: image_store of 16_16_16_16_FLOAT: texel (3, 0) component 2 is 32256, expected 32265
i.e. Metal stored 0x7e00 where round-toward-zero wants 0x7e09. So on this GPU the native path does not round toward zero, which is the case the RADV and AMD/NVIDIA-Windows runs above could not exercise, and the test does catch the difference.
agc_driver_sample_multisample_target is skipped here (MoltenVK does not expose shaderStorageImageMultisample), so the multisample-view regression seen earlier on NVIDIA does not manifest on this host — it is not re-tested, not fixed, on Apple. agc_shader_memory passes.
GPUs tested: Apple M2 Max (MoltenVK 1.4.2).
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Rerun at It builds as is now: the two-argument
So the multisample regression from
AI-assisted: yes |
|
Linux, lavapipe as in CI's Linux job: Mesa 26.2.4 llvmpipe (LLVM 23.1.1, 256 bits) here, Head. 25 of 26 pass in each of 3 plain and 6 validation runs. Red control, on a host where it fires. That slot is Validation. Head and red control alike: one message per run, Not this PR. AI-assisted: yes |
|
RADV rerun at RX 9070 XT (RDNA4, gfx1201), Mesa 26.2.4 RADV, Linux 7.3.0 (CachyOS), GCC, Release. This PR merged onto
So the 16-bit float store regression on RADV is gone on this head. Tested and written by Claude (AI assistant) on a contributor's machine. |
|
RADV / Polaris run at
AI-assisted: yes. |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
The change looks right and the hardware data in #8282 and the host runs above support it, but the branch now conflicts with main after #6218 (packed storage images merged). Please rebase and resolve these three places, then rerun the focused tests:
Graphics/src/Texture.cpp(StorageTexture::StorageView):mainnow picksPackedStorageViewFormat(storageFormat)first and falls back toUintFormatOfSint. Keep the packed route unchanged and applyuintStorageonly to the non-packed fallback, so the packed formats keep their view when the binding is outside the converted-float16 range.Optimization/src/ResourceMaterializer.cpp(inspectImageDescriptor):maincomputesSampledTextureNumericClass(remappedFormat).rawFloat16Storagehas to test the same remapped format thatmainnow uses, not the rawformat, otherwise a remapped descriptor takes a different path than the one the test covers.Graphics/src/MultisampleTarget.cpp(the storage-support check next to the newtwinFormatblock): keep both theuintStorageSupported()condition and thetwinFormatselection.
After the rebase, please post agc_driver_image_store_float_rounding and agc_driver_sample_multisample_target on the rebased head, plus the existing packed-storage tests from #6218, to show the two view routes still work together.
What
Use a UINT storage-image view for write-only 16-bit float formats and convert each stored component with the existing round-toward-zero float-to-half routine. This preserves negative values, infinities and NaNs, and saturates finite overflow as observed on the AMD hardware documented in #8282. Add execution coverage for 16_FLOAT, 16_16_FLOAT and 16_16_16_16_FLOAT, including edge values and component masks. The follow-up binds UINT-compatible views for these converted formats while retaining native SFLOAT views for float bindings. This update also binds a compatible UINT view for multisampled float16 targets and restores the existing two-argument StorageView call sites after rebasing on current main.
Tested
ninja -C build agc_driver_sample_multisample_target_testssucceeds on updated head7081834388.agc_driver_sample_multisample_targetstops before test initialization because this host CPU lacks BMI1, BMI2, LZCNT, MOVBE, AVX2 and FMA. The test therefore did not reach Vulkan validation.7081834388cleanly merged ontomainb00235dda0.agc_driver_sample_multisample_targetandagc_driver_image_store_float_roundingeach pass 3/3 without validation and 3/3 withVK_LAYER_KHRONOS_validation; the focused validation runs report 0VUID-lines. Adjacent native-MSAA, image conversion/repacking and depth-storage tests also pass 3/3 in both modes.708183438:agc_driver_image_store_float_roundingpasses 3/3. The multisample target test is skipped because MoltenVK does not exposeshaderStorageImageMultisample. With the production change reverted and the new test retained, the float16 rounding test fails at0x7e00where round-toward-zero expects0x7e09, confirming the host rounding difference this path handles.python3 tools/check_conventions.py --base upstream/mainpasses.python3 tools/pr_overlap.py --branch codex/f32-image-store-rtz-8282reports shared graphics files in other open PRs, including fix(agc): support packed 16-bit storage images #6218, perf(shader): deduplicate runtime image switch modes and base-level sample branches #8716 and feat(shader): support 32-bit image atomics through keyed image tables #8792; their topics are packed storage images, shader emitter performance and image atomics, respectively. No dependency is identified.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)