[PATCH v4 0/1] MR150: vkd3d-shader/hlsl: Ignore "unroll" attribute for loops.
-- v4: vkd3d-shader/hlsl: Ignore "unroll" attribute for loops. https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/150
From: Nikolay Sivov <nsivov(a)codeweavers.com> --- libs/vkd3d-shader/hlsl.c | 6 +-- libs/vkd3d-shader/hlsl.h | 2 +- libs/vkd3d-shader/hlsl.y | 84 +++++++++++++++++++++++++++++++++----- tests/hlsl-for.shader_test | 26 +++++++++++- 4 files changed, 103 insertions(+), 15 deletions(-) diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 54f3292b..7046404f 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -1291,13 +1291,13 @@ struct hlsl_ir_jump *hlsl_new_jump(struct hlsl_ctx *ctx, enum hlsl_ir_jump_type return jump; } -struct hlsl_ir_loop *hlsl_new_loop(struct hlsl_ctx *ctx, struct vkd3d_shader_location loc) +struct hlsl_ir_loop *hlsl_new_loop(struct hlsl_ctx *ctx, const struct vkd3d_shader_location *loc) { struct hlsl_ir_loop *loop; if (!(loop = hlsl_alloc(ctx, sizeof(*loop)))) return NULL; - init_node(&loop->node, HLSL_IR_LOOP, NULL, &loc); + init_node(&loop->node, HLSL_IR_LOOP, NULL, loc); hlsl_block_init(&loop->body); return loop; } @@ -1456,7 +1456,7 @@ static struct hlsl_ir_node *clone_loop(struct hlsl_ctx *ctx, struct clone_instr_ { struct hlsl_ir_loop *dst; - if (!(dst = hlsl_new_loop(ctx, src->node.loc))) + if (!(dst = hlsl_new_loop(ctx, &src->node.loc))) return NULL; if (!clone_block(ctx, &dst->body, &src->body, map)) { diff --git a/libs/vkd3d-shader/hlsl.h b/libs/vkd3d-shader/hlsl.h index 3e631dd9..271b1c84 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -1072,7 +1072,7 @@ struct hlsl_ir_store *hlsl_new_store_index(struct hlsl_ctx *ctx, const struct hl struct hlsl_ir_store *hlsl_new_store_component(struct hlsl_ctx *ctx, struct hlsl_block *block, const struct hlsl_deref *lhs, unsigned int comp, struct hlsl_ir_node *rhs); -struct hlsl_ir_loop *hlsl_new_loop(struct hlsl_ctx *ctx, struct vkd3d_shader_location loc); +struct hlsl_ir_loop *hlsl_new_loop(struct hlsl_ctx *ctx, const struct vkd3d_shader_location *loc); struct hlsl_ir_resource_load *hlsl_new_resource_load(struct hlsl_ctx *ctx, const struct hlsl_resource_load_params *params, const struct vkd3d_shader_location *loc); struct hlsl_ir_resource_store *hlsl_new_resource_store(struct hlsl_ctx *ctx, const struct hlsl_deref *resource, diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 3873a939..3899d23e 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -436,12 +436,63 @@ enum loop_type LOOP_DO_WHILE }; -static struct list *create_loop(struct hlsl_ctx *ctx, enum loop_type type, struct list *init, struct list *cond, - struct list *iter, struct list *body, struct vkd3d_shader_location loc) +static bool attribute_list_has_duplicates(const struct parse_attribute_list *attrs) +{ + unsigned int i, j; + + for (i = 0; i < attrs->count; ++i) + { + for (j = i + 1; j < attrs->count; ++j) + { + if (!strcmp(attrs->attrs[i]->name, attrs->attrs[j]->name)) + return true; + } + } + + return false; +} + +static struct list *create_loop(struct hlsl_ctx *ctx, enum loop_type type, const struct parse_attribute_list *attributes, + struct list *init, struct list *cond, struct list *iter, struct list *body, const struct vkd3d_shader_location *loc) { struct list *list = NULL; struct hlsl_ir_loop *loop = NULL; struct hlsl_ir_if *cond_jump = NULL; + unsigned int i; + + if (attribute_list_has_duplicates(attributes)) + { + hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_INVALID_SYNTAX, "Found duplicate attribute."); + return NULL; + } + + /* Ignore unroll(0) attribute, and any invalid attribute. */ + for (i = 0; i < attributes->count; ++i) + { + const struct hlsl_attribute *attr = attributes->attrs[i]; + if (!strcmp(attr->name, "unroll")) + { + if (attr->args_count) + { + hlsl_fixme(ctx, loc, "Unroll attribute with iteration count."); + return NULL; + } + else + { + FIXME("Loop unrolling is not implemented.\n"); + } + } + else + { + if (!strcmp(attr->name, "loop") + || !strcmp(attr->name, "fastopt") + || !strcmp(attr->name, "allow_uav_condition")) + { + hlsl_fixme(ctx, loc, "Unhandled attribute %s.", attr->name); + return NULL; + } + } + } if (!(list = make_empty_list(ctx))) goto oom; @@ -4033,6 +4084,7 @@ static void validate_texture_format_type(struct hlsl_ctx *ctx, struct hlsl_type %type <attr> attribute %type <attr_list> attribute_list +%type <attr_list> attribute_list_optional %type <boolval> boolean @@ -4327,6 +4379,14 @@ attribute_list: $$.attrs[$$.count++] = $2; } +attribute_list_optional: + %empty + { + $$.count = 0; + $$.attrs = NULL; + } + | attribute_list + func_declaration: func_prototype compound_statement { @@ -5294,22 +5354,26 @@ if_body: } loop_statement: - KW_WHILE '(' expr ')' statement + attribute_list_optional KW_WHILE '(' expr ')' statement { - $$ = create_loop(ctx, LOOP_WHILE, NULL, $3, NULL, $5, @1); + if (!($$ = create_loop(ctx, LOOP_WHILE, &$1, NULL, $4, NULL, $6, &@1))) + YYABORT; } - | KW_DO statement KW_WHILE '(' expr ')' ';' + | attribute_list_optional KW_DO statement KW_WHILE '(' expr ')' ';' { - $$ = create_loop(ctx, LOOP_DO_WHILE, NULL, $5, NULL, $2, @1); + if (!($$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $6, NULL, $3, &@1))) + YYABORT; } - | KW_FOR '(' scope_start expr_statement expr_statement expr_optional ')' statement + | attribute_list_optional KW_FOR '(' scope_start expr_statement expr_statement expr_optional ')' statement { - $$ = create_loop(ctx, LOOP_FOR, $4, $5, $6, $8, @1); + if (!($$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@1))) + YYABORT; hlsl_pop_scope(ctx); } - | KW_FOR '(' scope_start declaration expr_statement expr_optional ')' statement + | attribute_list_optional KW_FOR '(' scope_start declaration expr_statement expr_optional ')' statement { - $$ = create_loop(ctx, LOOP_FOR, $4, $5, $6, $8, @1); + if (!($$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@1))) + YYABORT; hlsl_pop_scope(ctx); } diff --git a/tests/hlsl-for.shader_test b/tests/hlsl-for.shader_test index ae7146c9..e0fdaa47 100644 --- a/tests/hlsl-for.shader_test +++ b/tests/hlsl-for.shader_test @@ -9,7 +9,7 @@ float4 main(float tex : texcoord) : sv_target { int i; float x = 0.0; - for (i = 0; i < 10; i++) + [unroll] for (i = 0; i < 10; i++) { x += i; if (tex > 0.5 && i == 5) @@ -26,3 +26,27 @@ todo draw quad probe ( 0, 0, 159, 480) rgba (10.0, 35.0, 0.0, 0.0) probe (161, 0, 479, 480) rgba (10.0, 38.0, 0.0, 0.0) probe (481, 0, 640, 480) rgba ( 5.0, 10.0, 0.0, 0.0) + +[pixel shader] +float4 main(float tex : texcoord) : sv_target +{ + int i; + float x = 0.0; + [unroll] [attr1] for (i = 0; i < 10; i++) + { + x += i; + } + return float4(i, x, 0.0, 0.0); +} + +[pixel shader fail] +float4 main(float tex : texcoord) : sv_target +{ + int i; + float x = 0.0; + [unroll] [unroll] for (i = 0; i < 10; i++) + { + x += i; + } + return float4(i, x, 0.0, 0.0); +} -- GitLab https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/150
Maybe we could just FIXME() for the "unroll" (no count) case? Or WARN()? Just so that we get some trace reminding us that we aren't doing the correct thing, even though it shouldn't matter most of the time.
Sure, I pushed that. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/150#note_29595
Maybe we could just FIXME() for the "unroll" (no count) case? Or WARN()? Just so that we get some trace reminding us that we aren't doing the correct thing, even though it shouldn't matter most of the time.
BTW, in which case is unrolling actually going to break something? -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/150#note_29759
On Wed Apr 12 15:31:44 2023 +0000, Giovanni Mascellani wrote:
Maybe we could just FIXME() for the "unroll" (no count) case? Or WARN()? Just so that we get some trace reminding us that we aren't doing the correct thing, even though it shouldn't matter most of the time. BTW, in which case is unrolling actually going to break something? Unrolling on itself is not going to, not unrolling with explicit iteration counter might break things.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/150#note_29760
Giovanni Mascellani (@giomasce) commented about libs/vkd3d-shader/hlsl.y:
}
loop_statement: - KW_WHILE '(' expr ')' statement + attribute_list_optional KW_WHILE '(' expr ')' statement { - $$ = create_loop(ctx, LOOP_WHILE, NULL, $3, NULL, $5, @1); + if (!($$ = create_loop(ctx, LOOP_WHILE, &$1, NULL, $4, NULL, $6, &@1))) + YYABORT;
I think there is the general sentiment that you should try to avoid aborting unless you really need. In this case it seems that you can just call `hlsl_error()` and return an empty list, so parsing can continue and other errors can be found in the same run. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/150#note_29763
On Wed Apr 12 15:34:15 2023 +0000, Nikolay Sivov wrote:
Unrolling on itself is not going to, not unrolling with explicit iteration counter might break things. Well, it changes the code (e.g. it could allow / prevent some optimizations) and that could mean that the shader gives somewhat different output when there are numerically unstable float computations involved.
Very unlikely to be of concern in practice of course, but leaving a trail in the log should be valuable if that ever happens. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/150#note_29767
On Wed Apr 12 15:40:36 2023 +0000, Giovanni Mascellani wrote:
I think there is the general sentiment that you should try to avoid aborting unless you really need. In this case it seems that you can just call `hlsl_error()` and return an empty list, so parsing can continue and other errors can be found in the same run. I'm not going to argue that there isn't, but this is the first time I hear about this. In (most?) cases we have hlsl_error() followed by yyabort. Do you mean that we need some message before aborting, or that not all cases are the same, and aborting in some places is fine. Excluding oom case.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/150#note_29769
I'm not going to argue that there isn't, but this is the first time I hear about this. In (most?) cases we have hlsl_error() followed by yyabort. Do you mean that we need some message before aborting, or that not all cases are the same, and aborting in some places is fine. Excluding oom case.
It's kind of a new thing, yeah. Note though that we do need the abort if we're ever going to return null. The better point is that we don't need to return NULL if there are duplicate attributes. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/150#note_29780
On Wed Apr 12 16:56:55 2023 +0000, Zebediah Figura wrote:
I'm not going to argue that there isn't, but this is the first time I hear about this. In (most?) cases we have hlsl_error() followed by yyabort. Do you mean that we need some message before aborting, or that not all cases are the same, and aborting in some places is fine. Excluding oom case. It's kind of a new thing, yeah. Note though that we do need the abort if we're ever going to return null. The better point is that we don't need to return NULL if there are duplicate attributes. The ultimate point of avoiding aborts, incidentally, is that we want to report as many error messages as possible, in order to be more useful as a compiler.
-- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/150#note_29781
Well, it changes the code (e.g. it could allow / prevent some optimizations) and that could mean that the shader gives somewhat different output when there are numerically unstable float computations involved.
Very unlikely to be of concern in practice of course, but leaving a trail in the log should be valuable if that ever happens.
Yeah, I don't think that's worth worrying about, or we'd never get anywhere. We're never going to quite implement the same logic as Windows. More saliently, though, we should be reporting an error on unrecognized attributes, or failing that, at least have a hlsl_fixme() for them. -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/150#note_29782
On Wed Apr 12 17:00:46 2023 +0000, Zebediah Figura wrote:
Well, it changes the code (e.g. it could allow / prevent some optimizations) and that could mean that the shader gives somewhat different output when there are numerically unstable float computations involved.
Very unlikely to be of concern in practice of course, but leaving a trail in the log should be valuable if that ever happens. Yeah, I don't think that's worth worrying about, or we'd never get anywhere. We're never going to quite implement the same logic as Windows. More saliently, though, we should be reporting an error on unrecognized attributes, or failing that, at least have a hlsl_fixme() for them. Yeah I'm not worried of that, a FIXME() or a WARN() for that particular case is more than enough, just to leave something in the logs.
Agreed that an hlsl_warning() on unknown attributes would be also nice and also match the one from hlsl_emit_bytecode(). -- https://gitlab.winehq.org/wine/vkd3d/-/merge_requests/150#note_29784
participants (5)
-
Giovanni Mascellani (＠giomasce) -
Matteo Bruni (＠Mystral)
-
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)