[PATCH v5 0/3] MR188: vkd3d-shader/tpf: Add support for emitting sample_l instructions
From what I can tell, the recent work on SampleBias/SampleLevel did all of the work for us, we just need to take the same framework and include the case for the `sample_l` instruction.
Tested with some shaders from the native Linux version of Little Racers STREET. -- v5: vkd3d-shader/tpf: For sample_l/sample_b, set lod swizzle to SCALAR. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/188
From: Ethan Lee <flibitijibibo(a)gmail.com> Currently marked TODO, since the SM4 emitter does not support sample_l yet. Signed-off-by: Ethan Lee <flibitijibibo(a)gmail.com> --- tests/sampler.shader_test | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/sampler.shader_test b/tests/sampler.shader_test index 2cb70a0b..51d63f16 100644 --- a/tests/sampler.shader_test +++ b/tests/sampler.shader_test @@ -33,6 +33,19 @@ float4 main() : sv_target draw quad probe all rgba (0.25, 0, 0.25, 0) +[pixel shader todo] +SamplerState s; +Texture2D t; + +float4 main() : sv_target +{ + return t.SampleLevel(s, float2(0.5, 0.5), 0.0); +} + +[test] +todo draw quad +todo probe all rgba (0.25, 0, 0.25, 0) + [pixel shader] SamplerState s; Texture2D t; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/188
From: Ethan Lee <flibitijibibo(a)gmail.com> This gets the SampleLevel() HLSL test to pass. Signed-off-by: Ethan Lee <flibitijibibo(a)gmail.com> --- libs/vkd3d-shader/tpf.c | 12 +++++++----- tests/sampler.shader_test | 6 +++--- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 3debe952..5d194709 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -3717,6 +3717,10 @@ static void write_sm4_sample(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffer instr.opcode = VKD3D_SM4_OP_SAMPLE; break; + case HLSL_RESOURCE_SAMPLE_LOD: + instr.opcode = VKD3D_SM4_OP_SAMPLE_LOD; + break; + case HLSL_RESOURCE_SAMPLE_LOD_BIAS: instr.opcode = VKD3D_SM4_OP_SAMPLE_B; break; @@ -3743,7 +3747,8 @@ static void write_sm4_sample(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffer sm4_src_from_deref(ctx, &instr.srcs[2], sampler, sampler->var->data_type, VKD3DSP_WRITEMASK_ALL); instr.src_count = 3; - if (load->load_type == HLSL_RESOURCE_SAMPLE_LOD_BIAS) + if (load->load_type == HLSL_RESOURCE_SAMPLE_LOD + || load->load_type == HLSL_RESOURCE_SAMPLE_LOD_BIAS) { sm4_src_from_node(&instr.srcs[3], load->lod.node, VKD3DSP_WRITEMASK_ALL); ++instr.src_count; @@ -4515,6 +4520,7 @@ static void write_sm4_resource_load(struct hlsl_ctx *ctx, break; case HLSL_RESOURCE_SAMPLE: + case HLSL_RESOURCE_SAMPLE_LOD: case HLSL_RESOURCE_SAMPLE_LOD_BIAS: if (!load->sampler.var) { @@ -4543,10 +4549,6 @@ static void write_sm4_resource_load(struct hlsl_ctx *ctx, write_sm4_gather(ctx, buffer, resource_type, &load->node, &load->resource, &load->sampler, coords, HLSL_SWIZZLE(W, W, W, W), texel_offset); break; - - case HLSL_RESOURCE_SAMPLE_LOD: - hlsl_fixme(ctx, &load->node.loc, "SM4 sample-LOD expression."); - break; } } diff --git a/tests/sampler.shader_test b/tests/sampler.shader_test index 51d63f16..62c71b39 100644 --- a/tests/sampler.shader_test +++ b/tests/sampler.shader_test @@ -33,7 +33,7 @@ float4 main() : sv_target draw quad probe all rgba (0.25, 0, 0.25, 0) -[pixel shader todo] +[pixel shader] SamplerState s; Texture2D t; @@ -43,8 +43,8 @@ float4 main() : sv_target } [test] -todo draw quad -todo probe all rgba (0.25, 0, 0.25, 0) +draw quad +probe all rgba (0.25, 0, 0.25, 0) [pixel shader] SamplerState s; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/188
From: Ethan Lee <flibitijibibo(a)gmail.com> Per Francisco Casas <fcasas(a)codeweavers.com>: I just realized one more thing. I think we are not properly following the specification for both sample_l and sample_b. In the Restrictions section of the documentation for sample_b it is mentioned that the last src register must use a single component selector if it is not a scalar immediate. This "select_component" is also ilustrated in the sample_l format: sample_l[_aoffimmi(u,v,w)] dest[.mask], srcAddress[.swizzle], srcResource[.swizzle], srcSampler, srcLOD.select_component Currently, we are not respecting that since sm4_src_from_node() sets src->swizzle_type = VKD3D_SM4_SWIZZLE_VEC4; by default. It should be VKD3D_SM4_SWIZZLE_SCALAR in this case... The disassembly should show a single component in the swizzle of the last src register. Signed-off-by: Ethan Lee <flibitijibibo(a)gmail.com> --- libs/vkd3d-shader/tpf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 5d194709..604593eb 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -3751,6 +3751,7 @@ static void write_sm4_sample(struct hlsl_ctx *ctx, struct vkd3d_bytecode_buffer || load->load_type == HLSL_RESOURCE_SAMPLE_LOD_BIAS) { sm4_src_from_node(&instr.srcs[3], load->lod.node, VKD3DSP_WRITEMASK_ALL); + instr.srcs[3].swizzle_type = VKD3D_SM4_SWIZZLE_SCALAR; ++instr.src_count; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/188
On Fri Apr 28 23:27:41 2023 +0000, Francisco Casas wrote:
I just realized one more thing. I think we are not properly following the specification for both sample_l and sample_b. In the **Restrictions** section of the documentation for sample_b it is mentioned that the last src register must use a single component selector if it is not a scalar immediate. This "select_component" is also ilustrated in the sample_l format: ``` sample_l[_aoffimmi(u,v,w)] dest[.mask], srcAddress[.swizzle], srcResource[.swizzle], srcSampler, srcLOD.select_component ``` Currently, we are not respecting that since sm4_src_from_node() sets ``` src->swizzle_type = VKD3D_SM4_SWIZZLE_VEC4; ``` by default. It should be VKD3D_SM4_SWIZZLE_SCALAR in this case. If I am not mistaken, a simple: ``` instr.srcs[3].swizzle_type = VKD3D_SM4_SWIZZLE_SCALAR; ``` after calling sm4_src_from_node() should do. The disassembly should show a single component in the swizzle of the last src register. Added this as a new commit on top - the disassembly before this commit shows a single component already, which made me think it might have been getting set elsewhere, but _after_ the change the binary output was different, so it might be a coincidence.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/188#note_31570
On Fri Apr 28 23:27:41 2023 +0000, Ethan Lee wrote:
Added this as a new commit on top - the disassembly before this commit shows a single component already, which made me think it might have been getting set elsewhere, but _after_ the change the binary output was different, so it might be a coincidence. For the record, before the patch, with the native dissasembler `fxc -dumpbin`, I was getting:
sample_l r0.xyzw, r0.xyxx, t0.xyzw, s0, r0.zzzz
and I see that our dissasembler shows the last bit as `r0.z`. So there is a discrepancy there. Anyways, after the patch, I get ``` sample_l r0.xyzw, r0.xyxx, t0.xyzw, s0, r0.z ``` with the native dissasembler. So it seems good now. :) -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/188#note_31571
This merge request was approved by Francisco Casas. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/188
For the record, before the patch, with the native dissasembler `fxc -dumpbin`, I was getting:
``` sample_l r0.xyzw, r0.xyxx, t0.xyzw, s0, r0.zzzz ```
and I see that our dissasembler shows the last bit as `r0.z`. So there is a discrepancy there.
That's mostly a consequence of sm1/d3dbc not having a distinction between VKD3D_SM4_SWIZZLE_SCALAR/VKD3D_SM4_SWIZZLE_VEC4; "r0.z" and "r0.zzzz" are equivalent there. See also shader_sm4_read_src_param() and shader_dump_src_param(). We should probably fix that, although there would be consequences for e.g. the SPIR-V backend as well. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/188#note_31636
participants (4)
-
Ethan Lee -
Ethan Lee (＠flibitijibibo)
-
Francisco Casas (＠fcasas)
-
Henri Verbeet (＠hverbeet)