[PATCH v3 0/5] MR358: vkd3d-shader/hlsl: Parse 'centroid' and 'noperspective' modifiers.
Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> -- v3: vkd3d-shader/tpf: Output interpolation modifiers for input declarations. vkd3d-shader/hlsl: Propagate structure fields modifiers when copying shader inputs. vkd3d-shader/hlsl: Allow interpolation modifiers on structure fields. vkd3d-shader/hlsl: Parse 'centroid' and 'noperspective' modifiers. tests: Add a test for interpolation modifiers specified on structure fields. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/358
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- tests/hlsl/nointerpolation.shader_test | 27 ++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/tests/hlsl/nointerpolation.shader_test b/tests/hlsl/nointerpolation.shader_test index 8f15be6ed..cd6720dba 100644 --- a/tests/hlsl/nointerpolation.shader_test +++ b/tests/hlsl/nointerpolation.shader_test @@ -25,3 +25,30 @@ float4 main(nointerpolation float4 t : texcoord) : sv_target [test] draw triangle list 3 probe all rgba (0.0, 1.0, 0.0, 1.0) + +[vertex shader] +struct ps_input +{ + nointerpolation float4 t : texcoord; +}; + +void main(uint id : sv_vertexid, inout ps_input input, out float4 pos : sv_position) +{ + float2 coords = float2((id << 1) & 2, id & 2); + pos = float4(coords * float2(2, -2) + float2(-1, 1), 0, 1); +} + +[pixel shader] +struct ps_input +{ + nointerpolation float4 t : texcoord; +}; + +float4 main(ps_input input) : sv_target +{ + return input.t; +} + +[test] +draw triangle list 3 +todo probe all rgba (0.0, 1.0, 0.0, 1.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/358
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.c | 4 ++++ libs/vkd3d-shader/hlsl.h | 2 ++ libs/vkd3d-shader/hlsl.l | 2 ++ libs/vkd3d-shader/hlsl.y | 10 ++++++++++ 4 files changed, 18 insertions(+) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 5fe9047bf..08714cc9e 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -2193,6 +2193,10 @@ struct vkd3d_string_buffer *hlsl_modifiers_to_string(struct hlsl_ctx *ctx, unsig vkd3d_string_buffer_printf(string, "extern "); if (modifiers & HLSL_STORAGE_NOINTERPOLATION) vkd3d_string_buffer_printf(string, "nointerpolation "); + if (modifiers & HLSL_MODIFIER_CENTROID) + vkd3d_string_buffer_printf(string, "centroid "); + if (modifiers & HLSL_MODIFIER_NOPERSPECTIVE) + vkd3d_string_buffer_printf(string, "noperspective "); if (modifiers & HLSL_MODIFIER_PRECISE) vkd3d_string_buffer_printf(string, "precise "); if (modifiers & HLSL_STORAGE_SHARED) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 44cebaaf6..91f8d9c61 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -357,6 +357,8 @@ struct hlsl_attribute #define HLSL_STORAGE_IN 0x00000800 #define HLSL_STORAGE_OUT 0x00001000 #define HLSL_MODIFIER_INLINE 0x00002000 +#define HLSL_MODIFIER_CENTROID 0x00004000 +#define HLSL_MODIFIER_NOPERSPECTIVE 0x00008000 #define HLSL_TYPE_MODIFIERS_MASK (HLSL_MODIFIER_PRECISE | HLSL_MODIFIER_VOLATILE | \ HLSL_MODIFIER_CONST | HLSL_MODIFIER_ROW_MAJOR | \ diff --git a/libs/vkd3d-shader/hlsl.l b/libs/vkd3d-shader/hlsl.l index e9ae3ccf3..90abd64a3 100644 --- a/libs/vkd3d-shader/hlsl.l +++ b/libs/vkd3d-shader/hlsl.l @@ -74,6 +74,7 @@ BlendState {return KW_BLENDSTATE; } break {return KW_BREAK; } Buffer {return KW_BUFFER; } cbuffer {return KW_CBUFFER; } +centroid {return KW_CENTROID; } compile {return KW_COMPILE; } const {return KW_CONST; } continue {return KW_CONTINUE; } @@ -95,6 +96,7 @@ inout {return KW_INOUT; } matrix {return KW_MATRIX; } namespace {return KW_NAMESPACE; } nointerpolation {return KW_NOINTERPOLATION; } +noperspective {return KW_NOPERSPECTIVE; } out {return KW_OUT; } packoffset {return KW_PACKOFFSET; } pass {return KW_PASS; } diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index fb6d485ea..5aae74bd6 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -4588,6 +4588,7 @@ static void validate_texture_format_type(struct hlsl_ctx *ctx, struct hlsl_type %token KW_BREAK %token KW_BUFFER %token KW_CBUFFER +%token KW_CENTROID %token KW_COLUMN_MAJOR %token KW_COMPILE %token KW_CONST @@ -4610,6 +4611,7 @@ static void validate_texture_format_type(struct hlsl_ctx *ctx, struct hlsl_type %token KW_MATRIX %token KW_NAMESPACE %token KW_NOINTERPOLATION +%token KW_NOPERSPECTIVE %token KW_OUT %token KW_PACKOFFSET %token KW_PASS @@ -5901,6 +5903,14 @@ var_modifiers: { $$ = add_modifiers(ctx, $2, HLSL_STORAGE_NOINTERPOLATION, &@1); } + | KW_CENTROID var_modifiers + { + $$ = add_modifiers(ctx, $2, HLSL_MODIFIER_CENTROID, &@1); + } + | KW_NOPERSPECTIVE var_modifiers + { + $$ = add_modifiers(ctx, $2, HLSL_MODIFIER_NOPERSPECTIVE, &@1); + } | KW_PRECISE var_modifiers { $$ = add_modifiers(ctx, $2, HLSL_MODIFIER_PRECISE, &@1); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/358
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.h | 3 +++ libs/vkd3d-shader/hlsl.y | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 91f8d9c61..9fb80dfba 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -364,6 +364,9 @@ struct hlsl_attribute HLSL_MODIFIER_CONST | HLSL_MODIFIER_ROW_MAJOR | \ HLSL_MODIFIER_COLUMN_MAJOR) +#define HLSL_INTERPOLATION_MODIFIERS_MASK (HLSL_STORAGE_NOINTERPOLATION | HLSL_MODIFIER_CENTROID | \ + HLSL_MODIFIER_NOPERSPECTIVE) + #define HLSL_MODIFIERS_MAJORITY_MASK (HLSL_MODIFIER_ROW_MAJOR | HLSL_MODIFIER_COLUMN_MAJOR) #define HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT 0 diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 5aae74bd6..0bd928b94 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -4954,7 +4954,7 @@ field: if (!(type = apply_type_modifiers(ctx, $2, &modifiers, true, &@1))) YYABORT; - if (modifiers & ~HLSL_STORAGE_NOINTERPOLATION) + if (modifiers & ~HLSL_INTERPOLATION_MODIFIERS_MASK) { struct vkd3d_string_buffer *string; -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/358
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- libs/vkd3d-shader/hlsl_codegen.c | 5 ++++- tests/hlsl/nointerpolation.shader_test | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index ed31efc3f..2156771c6 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -377,6 +377,8 @@ static void prepend_input_copy_recurse(struct hlsl_ctx *ctx, struct hlsl_block * for (i = 0; i < hlsl_type_element_count(type); ++i) { + unsigned int element_modifiers = modifiers; + if (type->class == HLSL_CLASS_ARRAY) { elem_semantic_index = semantic_index @@ -391,6 +393,7 @@ static void prepend_input_copy_recurse(struct hlsl_ctx *ctx, struct hlsl_block * semantic = &field->semantic; elem_semantic_index = semantic->index; loc = &field->loc; + element_modifiers |= field->storage_modifiers; } if (!(c = hlsl_new_uint_constant(ctx, i, &var->loc))) @@ -402,7 +405,7 @@ static void prepend_input_copy_recurse(struct hlsl_ctx *ctx, struct hlsl_block * return; list_add_after(&c->entry, &element_load->node.entry); - prepend_input_copy_recurse(ctx, block, element_load, modifiers, semantic, elem_semantic_index); + prepend_input_copy_recurse(ctx, block, element_load, element_modifiers, semantic, elem_semantic_index); } } else diff --git a/tests/hlsl/nointerpolation.shader_test b/tests/hlsl/nointerpolation.shader_test index cd6720dba..3b64f7069 100644 --- a/tests/hlsl/nointerpolation.shader_test +++ b/tests/hlsl/nointerpolation.shader_test @@ -51,4 +51,4 @@ float4 main(ps_input input) : sv_target [test] draw triangle list 3 -todo probe all rgba (0.0, 1.0, 0.0, 1.0) +probe all rgba (0.0, 1.0, 0.0, 1.0) -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/358
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- libs/vkd3d-shader/tpf.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 58b7f030d..01ec84574 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -4180,7 +4180,33 @@ static void write_sm4_dcl_semantic(const struct tpf_writer *tpf, const struct hl enum vkd3d_shader_interpolation_mode mode = VKD3DSIM_LINEAR; if ((var->storage_modifiers & HLSL_STORAGE_NOINTERPOLATION) || type_is_integer(var->data_type)) + { mode = VKD3DSIM_CONSTANT; + } + else + { + static const struct + { + unsigned int modifiers; + enum vkd3d_shader_interpolation_mode mode; + } + modes[] = + { + { HLSL_MODIFIER_CENTROID | HLSL_MODIFIER_NOPERSPECTIVE, VKD3DSIM_LINEAR_NOPERSPECTIVE_CENTROID }, + { HLSL_MODIFIER_NOPERSPECTIVE, VKD3DSIM_LINEAR_NOPERSPECTIVE }, + { HLSL_MODIFIER_CENTROID, VKD3DSIM_LINEAR_CENTROID }, + }; + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(modes); ++i) + { + if ((var->storage_modifiers & modes[i].modifiers) == modes[i].modifiers) + { + mode = modes[i].mode; + break; + } + } + } instr.opcode |= mode << VKD3D_SM4_INTERPOLATION_MODE_SHIFT; } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/358
Also, don't introduce the tests at the end of the MR. Rather, introduce tests at the beginning of the MR, mark them as `todo` and remove the `todo` marking when the tests start passing. This also makes the review easier. Please fix this second thing in this MR.
Done. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/358#note_46602
I can't say there are too many tests: 3/5 and 5/5 could also have some tests. But I'll say that the MR is good enough. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/358#note_46620
This merge request was approved by Giovanni Mascellani. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/358
On Fri Sep 22 16:24:19 2023 +0000, Nikolay Sivov wrote:
changed this line in [version 2 of the diff](/wine/vkd3d/-/merge_requests/358/diffs?diff_id=71144&start_sha=ed026e5b1ebd4020476d5c00e3b3d0c85ac18981#c6c9a40d72060550079c91bb6dd5741748169928_2194_2194) This is... technically addressed, but I'd say we should, along the same lines, call these HLSL_STORAGE_CENTROID and HLSL_STORAGE_NOPERSPECTIVE. HLSL_MODIFIER_* is only for those valid on typedefs.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/358#note_47528
On Fri Sep 22 16:27:34 2023 +0000, Nikolay Sivov wrote:
I repurposed existing tests to show that field modifiers are actually used. Please take a look. The test is structured how I was imagining, but what I'm ideally looking for is a test that shows this really should be |= (and not something more complicated). I.e. if you specify nointerpolation on the struct itself, is that propagated onto all struct fields? What if there's conflicting modifiers?
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/358#note_47529
On Tue Oct 3 23:24:18 2023 +0000, Zebediah Figura wrote:
The test is structured how I was imagining, but what I'm ideally looking for is a test that shows this really should be |= (and not something more complicated). I.e. if you specify nointerpolation on the struct itself, is that propagated onto all struct fields? What if there's conflicting modifiers? Yes, the rules are more complicated apparently.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/358#note_47749
On Thu Oct 5 20:35:07 2023 +0000, Nikolay Sivov wrote:
Yes, the rules are more complicated apparently. Or maybe they are not. The order of precedence is 'nointerpolation' -> 'sample' -> 'centroid' or 'noperspective'. Conflicts are tested only at the same level - on fields or variables.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/358#note_47783
participants (4)
-
Giovanni Mascellani (＠giomasce) -
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)