[PATCH v4 0/3] MR268: vkd3d-shader/hlsl: Handle vector arguments in fmod().
-- v4: tests: Add a test for fmod() with vector arguments. vkd3d-shader: Use ternary operator in fmod() implementation. vkd3d-shader/tpf: Use 'movc' to implement ternary operator. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/268
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.c | 2 ++ libs/vkd3d-shader/hlsl.h | 2 ++ libs/vkd3d-shader/hlsl.y | 6 +++- libs/vkd3d-shader/hlsl_codegen.c | 50 ++++++++++++++++++++++++++++++++ libs/vkd3d-shader/tpf.c | 25 ++++++++++++++++ 5 files changed, 84 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 8b706e1e..14944a2c 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -2467,6 +2467,8 @@ const char *debug_hlsl_expr_op(enum hlsl_ir_expr_op op) [HLSL_OP3_DP2ADD] = "dp2add", [HLSL_OP3_LERP] = "lerp", + [HLSL_OP3_MOVC] = "movc", + [HLSL_OP3_TERNARY] = "ternary", }; return op_names[op]; diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index dbeea1a0..951cd95d 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -548,6 +548,8 @@ enum hlsl_ir_expr_op HLSL_OP3_DP2ADD, HLSL_OP3_LERP, + HLSL_OP3_MOVC, + HLSL_OP3_TERNARY, }; #define HLSL_MAX_OPERANDS 3 diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 0d994673..8262fd40 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -6601,6 +6601,7 @@ conditional_expr: struct hlsl_ir_node *cond = node_from_block($1); struct hlsl_ir_node *first = node_from_block($3); struct hlsl_ir_node *second = node_from_block($5); + struct hlsl_ir_node *args[HLSL_MAX_OPERANDS] = { 0 }; struct hlsl_type *common_type; hlsl_block_add_block($1, $3); @@ -6617,7 +6618,10 @@ conditional_expr: if (!(second = add_implicit_conversion(ctx, $1, second, common_type, &@5))) YYABORT; - if (!hlsl_add_conditional(ctx, $1, cond, first, second)) + args[0] = cond; + args[1] = first; + args[2] = second; + if (!add_expr(ctx, $1, HLSL_OP3_TERNARY, args, common_type, &@1)) YYABORT; $$ = $1; } diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c index ec21b984..af508845 100644 --- a/libs/vkd3d-shader/hlsl_codegen.c +++ b/libs/vkd3d-shader/hlsl_codegen.c @@ -2357,6 +2357,54 @@ static bool lower_round(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void * return true; } +/* Use 'movc' for the ternary operator. */ +static bool lower_ternary(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context) +{ + struct hlsl_ir_node *operands[HLSL_MAX_OPERANDS], *replacement; + struct hlsl_ir_node *zero, *cond, *first, *second; + struct hlsl_constant_value zero_value = { 0 }; + struct hlsl_ir_expr *expr; + struct hlsl_type *type; + + if (instr->type != HLSL_IR_EXPR) + return false; + + expr = hlsl_ir_expr(instr); + if (expr->op != HLSL_OP3_TERNARY) + return false; + + cond = expr->operands[0].node; + first = expr->operands[1].node; + second = expr->operands[2].node; + + if (cond->data_type->base_type == HLSL_TYPE_FLOAT) + { + if (!(zero = hlsl_new_constant(ctx, cond->data_type, &zero_value, &instr->loc))) + return false; + list_add_tail(&instr->entry, &zero->entry); + + memset(operands, 0, sizeof(operands)); + operands[0] = zero; + operands[1] = cond; + type = cond->data_type; + type = hlsl_get_numeric_type(ctx, type->class, HLSL_TYPE_BOOL, type->dimx, type->dimy); + if (!(cond = hlsl_new_expr(ctx, HLSL_OP2_NEQUAL, operands, type, &instr->loc))) + return false; + list_add_before(&instr->entry, &cond->entry); + } + + memset(operands, 0, sizeof(operands)); + operands[0] = cond; + operands[1] = first; + operands[2] = second; + if (!(replacement = hlsl_new_expr(ctx, HLSL_OP3_MOVC, operands, first->data_type, &instr->loc))) + return false; + list_add_before(&instr->entry, &replacement->entry); + + hlsl_replace_node(instr, replacement); + return true; +} + static bool lower_casts_to_bool(struct hlsl_ctx *ctx, struct hlsl_ir_node *instr, void *context) { struct hlsl_type *type = instr->data_type, *arg_type; @@ -4317,6 +4365,8 @@ int hlsl_emit_bytecode(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry hlsl_transform_ir(ctx, lower_combined_samples, body, NULL); hlsl_transform_ir(ctx, track_object_components_usage, body, NULL); + if (profile->major_version >= 4) + hlsl_transform_ir(ctx, lower_ternary, body, NULL); if (profile->major_version < 4) { hlsl_transform_ir(ctx, lower_division, body, NULL); diff --git a/libs/vkd3d-shader/tpf.c b/libs/vkd3d-shader/tpf.c index 39c8b6f6..e6545f78 100644 --- a/libs/vkd3d-shader/tpf.c +++ b/libs/vkd3d-shader/tpf.c @@ -4610,10 +4610,31 @@ static void write_sm4_store_uav_typed(const struct tpf_writer *tpf, const struct write_sm4_instruction(tpf, &instr); } +static void write_sm4_movc(const struct tpf_writer *tpf, enum vkd3d_sm4_opcode opcode, + const struct hlsl_ir_node *dst, const struct hlsl_ir_node *src1, const struct hlsl_ir_node *src2, + const struct hlsl_ir_node *src3) +{ + struct sm4_instruction instr; + + memset(&instr, 0, sizeof(instr)); + instr.opcode = opcode; + + sm4_dst_from_node(&instr.dsts[0], dst); + instr.dst_count = 1; + + sm4_src_from_node(&instr.srcs[0], src1, instr.dsts[0].writemask); + sm4_src_from_node(&instr.srcs[1], src2, instr.dsts[0].writemask); + sm4_src_from_node(&instr.srcs[2], src3, instr.dsts[0].writemask); + instr.src_count = 3; + + write_sm4_instruction(tpf, &instr); +} + static void write_sm4_expr(const struct tpf_writer *tpf, const struct hlsl_ir_expr *expr) { const struct hlsl_ir_node *arg1 = expr->operands[0].node; const struct hlsl_ir_node *arg2 = expr->operands[1].node; + const struct hlsl_ir_node *arg3 = expr->operands[2].node; const struct hlsl_type *dst_type = expr->node.data_type; struct vkd3d_string_buffer *dst_type_string; @@ -5039,6 +5060,10 @@ static void write_sm4_expr(const struct tpf_writer *tpf, const struct hlsl_ir_ex &expr->node, arg1, arg2); break; + case HLSL_OP3_MOVC: + write_sm4_movc(tpf, VKD3D_SM4_OP_MOVC, &expr->node, arg1, arg2, arg3); + break; + default: hlsl_fixme(tpf->ctx, &expr->node.loc, "SM4 %s expression.", debug_hlsl_expr_op(expr->op)); } -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/268
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.y | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 8262fd40..6937899f 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -2830,6 +2830,7 @@ static bool intrinsic_fmod(struct hlsl_ctx *ctx, const struct parse_initializer const struct vkd3d_shader_location *loc) { struct hlsl_ir_node *x, *y, *div, *abs, *frac, *neg_frac, *ge, *select, *zero; + struct hlsl_ir_node *operands[HLSL_MAX_OPERANDS] = { 0 }; static const struct hlsl_constant_value zero_value; if (!(x = intrinsic_float_convert_arg(ctx, params, params->args[0], loc))) @@ -2857,7 +2858,10 @@ static bool intrinsic_fmod(struct hlsl_ctx *ctx, const struct parse_initializer if (!(ge = add_binary_comparison_expr(ctx, params->instrs, HLSL_OP2_GEQUAL, div, zero, loc))) return false; - if (!(select = hlsl_add_conditional(ctx, params->instrs, ge, frac, neg_frac))) + operands[0] = ge; + operands[1] = frac; + operands[2] = neg_frac; + if (!(select = add_expr(ctx, params->instrs, HLSL_OP3_TERNARY, operands, x->data_type, loc))) return false; return !!add_binary_arithmetic_expr(ctx, params->instrs, HLSL_OP2_MUL, select, y, loc); -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/268
From: Nikolay Sivov <nsivov(a)codeweavers.com> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com> --- tests/hlsl/fmod.shader_test | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/hlsl/fmod.shader_test b/tests/hlsl/fmod.shader_test index 4c9ff988..9eb69c3b 100644 --- a/tests/hlsl/fmod.shader_test +++ b/tests/hlsl/fmod.shader_test @@ -11,3 +11,17 @@ probe all rgba (-0.5, 0.0, 0.0, 0.0) 4 uniform 0 float4 1.1 0.3 0.0 0.0 draw quad probe all rgba (0.2, 0.0, 0.0, 0.0) 4 + +[pixel shader] +float4 main(uniform float4 u) : sv_target +{ + return float4(fmod(u.xy, u.z), 0, 0); +} + +[test] +uniform 0 float4 -0.5 6.5 2.0 0.0 +draw quad +probe all rgba (-0.5, 0.5, 0.0, 0.0) 4 +uniform 0 float4 1.1 0.3 3.0 0.0 +draw quad +probe all rgba (1.1, 0.3, 0.0, 0.0) 4 -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/268
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.h:
HLSL_OP3_DP2ADD, HLSL_OP3_LERP, + HLSL_OP3_MOVC, + HLSL_OP3_TERNARY,
Maybe we should add a comment explaining the difference and rationale behind having both `MOVC` and `TERNARY`. Even for us it took a little bit of back and forth to be understood, so that's probably for the random code reader as well. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/268#note_42438
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/tpf.c:
write_sm4_instruction(tpf, &instr); }
+static void write_sm4_movc(const struct tpf_writer *tpf, enum vkd3d_sm4_opcode opcode,
This should probably be called `write_sm4_ternary_op()` and placed just below `write_sm4_binary_op_with_two_destinations()`. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/268#note_42440
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
if (!(second = add_implicit_conversion(ctx, $1, second, common_type, &@5))) YYABORT;
- if (!hlsl_add_conditional(ctx, $1, cond, first, second)) + args[0] = cond; + args[1] = first; + args[2] = second; + if (!add_expr(ctx, $1, HLSL_OP3_TERNARY, args, common_type, &@1))
This changes the behavior of the conditional expression (in two different ways, unless I am mistaken: because the check is now done per component and because negative zero is considered false). So it would be good to have a test that shows that the change is correct. As usual, the test should be introduced with `todo` first, then fixed in a later commit. Also, while we are here, it would be nice to check what requirements native imposes on the shape of the condition argument and enforce them as well. Currently we don't check anything on the condition, we just perform the usual implicit conversion to the common type for the "if-true" and "if-false" arguments. I'd guess that native is a bit more picky. We should also check what happens if the conditional operator is fed an object type. Internally I guess that we want the three arguments to have the same shape (and assert that in `lower_ternary()` and in whatever SM1 will have), also because we want `add_expr()` to perform matrix splitting appropriately. The second part (about checking types) doesn't need to be in this MR, but it would be nice if you could address it at some point. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/268#note_42439
This changes the behavior of the conditional expression (in two different ways, unless I am mistaken: because the check is now done per component and because negative zero is considered false). So it would be good to have a test that shows that the change is correct. As usual, the test should be introduced with `todo` first, then fixed in a later commit.
This will need SM1 case fixed first, to avoid the need of running tests on SM4+ only. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/268#note_42613
On Wed Aug 16 08:52:44 2023 +0000, Giovanni Mascellani wrote:
Maybe we should add a comment explaining the difference and rationale behind having both `MOVC` and `TERNARY`. Even for us it took a little bit of back and forth to be understood, so that's probably for the random code reader as well. I think it will be clear once older profile cases are implemented.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/268#note_42614
On Thu Aug 17 14:46:16 2023 +0000, Nikolay Sivov wrote:
This changes the behavior of the conditional expression (in two different ways, unless I am mistaken: because the check is now done per component and because negative zero is considered false). So it would be good to have a test that shows that the change is correct. As usual, the test should be introduced with `todo` first, then fixed in a later commit. This will need SM1 case fixed first, to avoid the need of running tests on SM4+ only. Probably does not have to wait for this, we don't depend on it yet when running tests on Linux.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/268#note_42615
participants (3)
-
Giovanni Mascellani (＠giomasce) -
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)