vkd3d-shader/spirv: Implement MAD in two operations if flagged as precise.
This would eliminate the todo for the precise mad() test in !718 (merged). Maybe we need test results on nvidia and intel to decide if we actually want this.
Merge request reports
Activity
That todo succeeds on all the hardware I managed to test with once I cherry pick this on top of !718 (merged). I tested:
- RADV,
- Intel,
- NVIDIA proprietary,
- llvmpipe.
So I would accept this after !718 (merged) is in and removing the todo.
added 25 commits
-
0ceeec95...164608a0 - 24 commits from branch
wine:master
- 4860d22c - vkd3d-shader/spirv: Implement MAD in two operations if flagged as precise.
-
0ceeec95...164608a0 - 24 commits from branch
added 1 commit
- ec46b7a6 - vkd3d-shader/spirv: Implement MAD in two operations if flagged as precise.
I suspect this MR will need a rebase in any case.
+ /* The HLSL docs state: "If components of a mad instruction are tagged as precise, the + * hardware must execute a mad instruction or the exact equivalent, and it cannot split + * it into a multiply followed by an add." + * But DXIL.rst states the opposite: "Floating point multiply & add. This operation is + * not fused for "precise" operations." + * Windows drivers seem to conform with the latter, for SM 4-5 and SM 6. */
The comment seems a bit unfortunate/ambiguous. The "HLSL docs" here refer to the "Shader Model 5 Assembly" documentation. (As opposed to e.g. referring to the translation of the HLSL mad() intrinsic to d3dbc/tpf.)
Do we want to handle this in the SPIR-V backend? It seems like something vsir_program_normalise() could take care of, possibly as part of the current vsir_program_lower_texkills() pass.
added 84 commits
-
ec46b7a6...9c0d04c8 - 83 commits from branch
wine:master
- 251d8521 - vkd3d-shader/spirv: Implement MAD in two operations if flagged as precise.
-
ec46b7a6...9c0d04c8 - 83 commits from branch
added 1 commit
- 092f7958 - vkd3d-shader/ir: Implement MAD in two operations if flagged as precise.
added 1 commit
- 4f98ef63 - vkd3d-shader/ir: Implement MAD in two operations if flagged as precise.
added 1 commit
- 86b55c1c - vkd3d-shader/ir: Implement MAD in two operations if flagged as precise.
I don't think the flattener should be "hijacked" for this pass, it is already complicated enough. It's probably appropriate to have a single pass to collect all the little local operations like this one, instead of iterating over the whole instruction array every time, but it should be another one. As Henri notices, the same pass might do this and lowering texkills (and probably removing DCL_TEMPS too).
I added it to
lower_texkills()
. Moving instructions for each occurrence takes quadratic time, so we could see pathological cases. For example, Cyberpunk has a colossal compute shader with over 40,000 SSA values, so a shader of similar size which uses preciseMAD
could occur. If necessary we could convertlower_texkills_and_precise_mad()
to copy instead of insert.Yeah, I still don't like the way that pass is structured and I think it would be a better idea to rewrite it in terms of a full program copy like many other passes are structured. But that's an independent issue that might be addressed at some point: in terms of software design I think it's better to lump all these smaller changes to the shader in a single pass.
However, what do you think of refactoring your MR in a manner similar to this (last three commits)?
Sorry for the double comment, but let me add another couple of remarks:
- At some point it might happen that the instruction written by one of the helpers called by
vsir_program_lower_instruction()
should itself be processed again. To handle this we could have the switch cases returnVKD3D_OK
when they did something andVKD3D_FALSE
when they touched nothing, so that the loop invsir_program_lower_instruction()
can decide whether to increment the position or not. - Both the TEXKILL and the MAD helpers currently use a TEMP for storing the intermediate values. This should probably become a SSA at some point; even for SM4 there shouldn't be problem for passes to introduce SSAs, provided that they are written only once and the definition point dominates the usage points. The SPIR-V backend should handle them just fine. This has the advantage of not requiring to pass a pointer to all the helpers to coordinate the TEMP index, and I guess in general the less we use TEMPs the better.
- At some point it might happen that the instruction written by one of the helpers called by
Your branch looks good to me. Would you like to submit a new MR with those three commits, while I close mine? A couple of comments: there's a typo in the commit message for DCL_TEMPS, and I would rename
vsir_program_lower_mad
tovsir_program_lower_precise_mad
for accuracy.To handle this we could have the switch cases return
VKD3D_OK
when they did something andVKD3D_FALSE
when they touched nothingThat makes sense if the need arises.
This should probably become a SSA at some point
Yes, and the coordinates and texels which the DXIL parser emits should probably be changed at the same time, though we would need a COMPOSITE_CONSTRUCT instruction to make those SSA. Maybe after the release.
Ok, submitted at !779 (merged).
added 1 commit
- a04cf97b - vkd3d-shader/ir: Implement MAD in two operations if flagged as precise.
Yeah, I still don't like the way that pass is structured and I think it would be a better idea to rewrite it in terms of a full program copy like many other passes are structured.
I broadly disagree. TEXKILL isn't that common, and all this pre-emptive copying that a number of these passes do can't possibly be cheaper than an efficient implementation of shader_instruction_array_insert_at() would be.
The existing implementation of shader_instruction_array_insert_at() certainly isn't optimal, but that's an issue that can be addressed there; it's not a problem with the passes using it.
But that's an independent issue that might be addressed at some point: in terms of software design I think it's better to lump all these smaller changes to the shader in a single pass.
However, what do you think of refactoring your MR in a manner similar to this (last three commits)?
I didn't review those commits in detail, but in terms of the structure, that's pretty much exactly what I had in mind.
mentioned in merge request !779 (merged)