-- v9: vkd3d-shader/hlsl: Ignore "unroll" attribute for loops.
From: Nikolay Sivov nsivov@codeweavers.com
--- libs/vkd3d-shader/hlsl.c | 6 +-- libs/vkd3d-shader/hlsl.h | 2 +- libs/vkd3d-shader/hlsl.y | 76 +++++++++++++++++++++++++++++++++----- tests/hlsl-for.shader_test | 30 ++++++++++++++- 4 files changed, 99 insertions(+), 15 deletions(-)
diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c index 869638a6..d87be198 100644 --- a/libs/vkd3d-shader/hlsl.c +++ b/libs/vkd3d-shader/hlsl.c @@ -1392,13 +1392,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; } @@ -1557,7 +1557,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 b38fba1c..5da78b3e 100644 --- a/libs/vkd3d-shader/hlsl.h +++ b/libs/vkd3d-shader/hlsl.h @@ -1090,7 +1090,7 @@ bool hlsl_index_is_noncontiguous(struct hlsl_ir_index *index);
struct hlsl_ir_node *hlsl_new_index(struct hlsl_ctx *ctx, struct hlsl_ir_node *val, struct hlsl_ir_node *idx, const struct vkd3d_shader_location *loc); -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 0ddae6ee..e40ca447 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -436,12 +436,59 @@ 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."); + + /* 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_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_NOT_IMPLEMENTED, "Unroll attribute with iteration count."); + } + 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); + } + else + { + hlsl_warning(ctx, loc, VKD3D_SHADER_ERROR_HLSL_NOT_IMPLEMENTED, "Unrecognized attribute %s.", attr->name); + } + }
if (!(list = make_empty_list(ctx))) goto oom; @@ -4024,6 +4071,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
@@ -4318,6 +4366,14 @@ attribute_list: $$.attrs[$$.count++] = $2; }
+attribute_list_optional: + %empty + { + $$.count = 0; + $$.attrs = NULL; + } + | attribute_list + func_declaration: func_prototype compound_statement { @@ -5285,22 +5341,22 @@ 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); + $$ = create_loop(ctx, LOOP_WHILE, &$1, NULL, $4, NULL, $6, &@2); } - | 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); + $$ = create_loop(ctx, LOOP_DO_WHILE, &$1, NULL, $6, NULL, $3, &@2); } - | 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); + $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@2); 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); + $$ = create_loop(ctx, LOOP_FOR, &$1, $5, $6, $7, $9, &@2); hlsl_pop_scope(ctx); }
diff --git a/tests/hlsl-for.shader_test b/tests/hlsl-for.shader_test index ae7146c9..b6896981 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,31 @@ 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); +} + +[test] +draw quad +probe all rgba (10.0, 45.0, 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); +}
+ if (!strcmp(attr->name, "unroll")) + { + if (attr->args_count) + { + hlsl_error(ctx, loc, VKD3D_SHADER_ERROR_HLSL_NOT_IMPLEMENTED, "Unroll attribute with iteration count."); + } + else + { + FIXME("Loop unrolling is not implemented.\n"); + } + }
vkd3d-shader/hlsl has been fairly decent about using the proper channels to report compilation issues; why are we introducing a FIXME here instead of using e.g. hlsl_warning()?
On Tue Apr 18 12:21:16 2023 +0000, Henri Verbeet wrote:
+ if (!strcmp(attr->name, "unroll")) + { + if (attr->args_count) + { + hlsl_error(ctx, loc,VKD3D_SHADER_ERROR_HLSL_NOT_IMPLEMENTED, "Unroll attribute with iteration count.");
}else{FIXME("Loop unrolling is not implemented.\n");}}vkd3d-shader/hlsl has been fairly decent about using the proper channels to report compilation issues; why are we introducing a FIXME here instead of using e.g. hlsl_warning()?
My interpretation is that `hlsl_warning()` is for what the shader writer should change, while `FIXME()` is for what we should change. In this case, it is us who have to fix our code. I wouldn't use `hlsl_fixme()` either, because that kind of means "We have to fix our code, but since we don't want to give the client invalid code we also pretend there was a compilation error". Here we're not miscompiling, assuming that the fact that loops are unrolled or not is an implementation detail, so to me `FIXME()` really feels the best alternative.
My interpretation is that `hlsl_warning()` is for what the shader writer should change, while `FIXME()` is for what we should change. In this case, it is us who have to fix our code. I wouldn't use `hlsl_fixme()` either, because that kind of means "We have to fix our code, but since we don't want to give the client invalid code we also pretend there was a compilation error". Here we're not miscompiling, assuming that the fact that loops are unrolled or not is an implementation detail, so to me `FIXME()` really feels the best alternative.
I have two thoughts on that:
- ERR/FIXME/WARN/TRACE are primarily debug tools. For the purpose of reporting something to users they're really a bit of a last resort. In CLI applications like vkd3d-compiler, these will miss location information, and will be out of sync with other potential messages from the application. In GUI applications or vkd3d builds with VKD3D_NO_DEBUG_MESSAGES defined, these may not be visible at all.
- Presumably the user intentionally added the "unroll" attribute, so they may want to know that we're ignoring it. It's one thing to ignore a particular attribute, but quite another to do so silently.
- Presumably the user intentionally added the "unroll" attribute, so they may want to know that we're ignoring it. It's one thing to ignore a particular attribute, but quite another to do so silently.
On those grounds I suppose we could add a fixme whose severity is warning rather than error.
-
Giovanni Mascellani (＠giomasce) -
Henri Verbeet (＠hverbeet)
-
Nikolay Sivov
-
Nikolay Sivov (＠nsivov)
-
Zebediah Figura (＠zfigura)