Repository navigation
Conversation
|
Sorry for the slow reply, busy day. Here is the Palworld A/B for this head. Result: no measurable change in this scene. The three heaviest shaders take the same GPU time per call with and without it, and presents per 10 s stay within run-to-run variation. Setup
GPU time per call (
All three are 32x1x1 or 64x1x1 compute shaders dispatched once per frame. The packed ops are about 30% of shader 1's GCN instructions (45 Why it doesn't show here: what the special-value rules cost is small next to the rest of the lowering.
So in Palworld's heavy shaders the f32 denormal and NaN handling and the operand repacking weigh more than the f16 rules this PR guards. The guard itself works as described. I have no counter for how often lanes take the slow path in this scene. AI-assisted (Claude Code) |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
|
|
Thanks for the controlled A/B and for recording the setup, workload and test results. I read this as no measurable improvement in the Palworld title-screen scene: the three heaviest shaders and presents per 10 s stayed within run-to-run variation. The correctness tests pass, but this run does not demonstrate a gameplay performance gain, so I will not cite it as one. It also found no regression in this measured scene.\n\nI will keep the PR’s claim limited to avoiding unnecessary packed-f16 special-value repairs in the guarded cases; broader performance benefit still needs a workload that exercises those cases often enough to measure. |
CnRJay
left a comment
There was a problem hiding this comment.
Going by queeeef's A/B above, the trade-off here is negative. Shader 1 grows from 207,832 to 242,434 SPIR-V words (+17%), and GPU time per dispatch is unchanged within noise on an RTX 2080 Ti. A larger module means longer vkCreate*Pipelines for every shader that uses packed f16, which turns into hitches on first use for everyone, while the run-time gain doesn't show.
This is the same pattern as #4322/#5400. Guarding the rare path with a branch lets the GPU skip a few ALU ops per lane, but it pays in module size and control flow. Driver compilers often flatten such short branches back into selects, so the GPU saving may not exist on the final ISA. The repair sequence is a handful of integer ops, which GPUs run at full rate, so it is rarely the bottleneck. queeeef's breakdown points the same way: the operand repacking and the f32 denormal/NaN wrapping dominate these shaders, not the f16 rules.
If this is pursued, I'd suggest:
- measuring pipeline creation time for those three shaders, base vs. change, on NVIDIA and RADV, alongside the GPU time;
- or a version that keeps the repair as straight-line selects, removes the duplicated
op_selrepacking queeeef found (operands computed twice per op), and shrinks the module rather than growing it.
Without one of those, I don't think this should go in as perf(.
|
Thanks for the review. I agree the current evidence does not demonstrate a runtime win: the RTX 2080 Ti A/B is within noise, while shader 1 grows from 207,832 to 242,434 SPIR-V words. The test pass establishes the guarded translation's correctness, not its performance benefit. I also agree pipeline-creation time is the missing measurement before keeping a |
|
Thanks, that's a fair summary. One direction you can verify without a GPU: the duplicated |
|
Closing: as you noted above, the measurements so far show no gain, and the change affects every packed-f16 shader. Reopen with before/after numbers from one title scene if a gain turns up. |
|
I carried the reviewer suggestion about duplicate |
What
Closes #4042. The packed
v_pk_add_f16,v_pk_mul_f16, andv_pk_fma_f16/v_pk_fmac_f16translations currently emit the exact special-value repair sequence for every lane, including ordinary finite inputs. This moves the existing bit-selection rules behind a per-instruction guard: input/result NaNs and, where the native add/multiply path needs it, enabled overflow-to-infinity cases enter the unchanged repair path. Ordinary finite results use the existing fast result directly. The repair code itself is preserved.The regression case forces every lane through the slow path and compares the output bit-for-bit with the existing f32 rule path. It also checks ordinary randomized inputs through the existing native-vs-emulated comparison.
This shares the packed-f16 translation function with #4072. This branch retains the current f32 FMA lowering; if #4072 is integrated first, its native-FMA result path will need the same guard before this change is considered complete. No native-GPU or Palworld performance result is claimed here; the #4042 reporter offered to run that A/B measurement.
Tested
Linux, Release, lavapipe (Mesa). Built
agc_driver_packed_float16_native_tests,agc_driver_packed_alu_tests,agc_driver_float16_fma_rounding_tests, andagc_driver_fp16_overflow_tests.ctestpassed all four focused tests.python3 tools/check_conventions.py --base upstream/mainandgit diff --checkpassed. Palworld performance was not measured in this environment.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)