Signed-off-by: Francisco Casas fcasas@codeweavers.com --- Makefile.am | 1 - libs/vkd3d-shader/hlsl.y | 72 ++++++++++++++++++++++++++++++++++------ 2 files changed, 61 insertions(+), 12 deletions(-)
diff --git a/Makefile.am b/Makefile.am index d15e50c3..1086d028 100644 --- a/Makefile.am +++ b/Makefile.am @@ -298,7 +298,6 @@ XFAIL_TESTS = \ tests/hlsl-initializer-flattening.shader_test \ tests/hlsl-initializer-invalid-n-args.shader_test \ tests/hlsl-initializer-local-array.shader_test \ - tests/hlsl-initializer-numeric.shader_test \ tests/hlsl-initializer-static-array.shader_test \ tests/hlsl-initializer-struct.shader_test \ tests/hlsl-bool-cast.shader_test \ diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 988e0743..a89e4432 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1278,6 +1278,54 @@ static bool add_increment(struct hlsl_ctx *ctx, struct list *instrs, bool decrem return true; }
+static void numeric_var_initializer(struct hlsl_ctx *ctx, struct hlsl_ir_var *var, + struct parse_variable_def *v, unsigned int reg_offset, struct hlsl_type *type, + unsigned int *initializer_i, struct list *instrs) +{ + unsigned int writemask_offset = 0; + unsigned int components_read = 0; + + if (type->type == HLSL_CLASS_MATRIX) + hlsl_fixme(ctx, &var->loc, "Matrix initializer\n"); + + while (components_read < hlsl_type_component_count(type)) + { + struct hlsl_ir_store *store; + struct hlsl_ir_constant *c; + struct hlsl_ir_node *node; + unsigned int width; + + assert(*initializer_i < v->initializer.args_count); + node = v->initializer.args[*initializer_i]; + *initializer_i += 1; + + width = hlsl_type_component_count(node->data_type); + components_read += width; + + if (components_read > hlsl_type_component_count(type)) + { + hlsl_error(ctx, &node->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT, + "Initializer argument has spare components.\n"); + return; + } + + if (!(node = add_implicit_conversion(ctx, instrs, node, + hlsl_get_vector_type(ctx, type->base_type, width), &node->loc))) + return; + + if (!(c = hlsl_new_uint_constant(ctx, reg_offset, node->loc))) + return; + list_add_tail(instrs, &c->node.entry); + + if (!(store = hlsl_new_store(ctx, var, &c->node, node, + ((1u << width) - 1) << writemask_offset, node->loc))) + return; + list_add_tail(instrs, &store->node.entry); + + writemask_offset += width; + } +} + static void struct_var_initializer(struct hlsl_ctx *ctx, struct list *list, struct hlsl_ir_var *var, struct parse_initializer *initializer) { @@ -1449,7 +1497,6 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t if (v->initializer.args_count) { unsigned int size = initializer_size(&v->initializer); - struct hlsl_ir_load *load;
if (type->type <= HLSL_CLASS_LAST_NUMERIC && type->dimx * type->dimy != size && size != 1) @@ -1473,7 +1520,6 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t vkd3d_free(v); continue; } - if (type->type == HLSL_CLASS_STRUCT) { struct_var_initializer(ctx, statements_list, var, &v->initializer); @@ -1495,23 +1541,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t vkd3d_free(v); continue; } - if (v->initializer.args_count > 1) + + if (type->type <= HLSL_CLASS_LAST_NUMERIC && size == 1) { - hlsl_fixme(ctx, &v->loc, "Complex initializer."); - free_parse_initializer(&v->initializer); - vkd3d_free(v); - continue; + struct hlsl_ir_load *load = hlsl_new_var_load(ctx, var, var->loc); + + list_add_tail(v->initializer.instrs, &load->node.entry); + add_assignment(ctx, v->initializer.instrs, &load->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]); } + else + { + unsigned int initializer_i = 0;
- load = hlsl_new_var_load(ctx, var, var->loc); - list_add_tail(v->initializer.instrs, &load->node.entry); - add_assignment(ctx, v->initializer.instrs, &load->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]); - vkd3d_free(v->initializer.args); + numeric_var_initializer(ctx, var, v, 0, type, &initializer_i, v->initializer.instrs); + }
if (modifiers & HLSL_STORAGE_STATIC) list_move_tail(&ctx->static_initializers, v->initializer.instrs); else list_move_tail(statements_list, v->initializer.instrs); + + vkd3d_free(v->initializer.args); vkd3d_free(v->initializer.instrs); }
Hi,
this patch causes a regression on this shader:
--- float4 main() : SV_TARGET { float3 x = float4(71, 72, 73, 74); return float4(x, 74); } ---
This is accepted by native compiler and is works before your patch, but is broken after.
(see below)
On 10/01/22 20:33, Francisco Casas wrote:
@@ -1495,23 +1541,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t vkd3d_free(v); continue; }
if (v->initializer.args_count > 1)
if (type->type <= HLSL_CLASS_LAST_NUMERIC && size == 1) {
hlsl_fixme(ctx, &v->loc, "Complex initializer.");free_parse_initializer(&v->initializer);vkd3d_free(v);continue;
struct hlsl_ir_load *load = hlsl_new_var_load(ctx, var, var->loc);list_add_tail(v->initializer.instrs, &load->node.entry);add_assignment(ctx, v->initializer.instrs, &load->node, ASSIGN_OP_ASSIGN, v->initializer.args[0]); }
In other words, it seems that here you have to test for "v->initializer.args_count == 1", not "size == 1". But do your own research.
Also, consider adding something similar to my example above to the tests (in the spirit of "everything that was gotten wrong at least once is a good candidate for a test").
Thanks, Giovanni.
Hi again,
On 12/01/22 17:35, Giovanni Mascellani wrote:
In other words, it seems that here you have to test for "v->initializer.args_count == 1", not "size == 1". But do your own research.
Actually, I noticed that this shader does not compile on native:
--- float4 main() : SV_TARGET { float3 x = {float4(71, 72, 73, 74)}; return float4(x, 74); } ---
(the only difference with my previous email is the pair of braces around the initializer)
So it seems that the condition you want to discriminate on is whether the initializer is specified as a list (in which case you have to fully unpack and match in order) or as a single object (in which case you treat it as an assignment). I guess, at least.
However, I think we sometimes tolerate being more permissive than native, so maybe you can ignore this corner case. Senior developers will tell.
Giovanni.
January 12, 2022 1:48 PM, "Giovanni Mascellani" gmascellani@codeweavers.com wrote:
Hi again,
On 12/01/22 17:35, Giovanni Mascellani wrote:
In other words, it seems that here you have to test for > "v->initializer.args_count == 1", not "size == 1". But do your own > research.
Actually, I noticed that this shader does not compile on native:
float4 main() : SV_TARGET { float3 x = {float4(71, 72, 73, 74)}; return float4(x, 74); }
(the only difference with my previous email is the pair of braces around the initializer)
So it seems that the condition you want to discriminate on is whether the initializer is specified as a list (in which case you have to fully unpack and match in order) or as a single object (in which case you treat it as an assignment). I guess, at least.
No, I think that the original purpose of doing an assignment when "v->initializer.args_count == 1" is to cover the cases for when numeric variables are initialized with a single numerical constant.
For instance, this shader:
--- float4 main() : sv_target { float3 aa = 3; float4 bb = {aa.x, aa.y, aa.z, 4.0}; return bb; } ---
Retrieves (3.0, 3.0, 3.0, 4.0) with the native compiler.
If I am not mistaken, initializers like float4(1 ,2 ,3 ,4) are treated the same as {1, 2, 3, 4} in the code.
Still, I will add these tests in v3.
Sorry, I was flying low when I wrote my previous answer, so I will correct some things:
January 12, 2022 2:15 PM, "Francisco Casas" fcasas@codeweavers.com wrote:
January 12, 2022 1:48 PM, "Giovanni Mascellani" gmascellani@codeweavers.com wrote:
Hi again,
On 12/01/22 17:35, Giovanni Mascellani wrote:
In other words, it seems that here you have to test for > "v->initializer.args_count == 1", not "size == 1". But do your own > research.
This was correct, **I** was thinking on the single numeric initializer case, but replacing "size == 1" with "v->initializer.args_count == 1", besides preempting the regression, covers this case.
Actually, I noticed that this shader does not compile on native:
float4 main() : SV_TARGET { float3 x = {float4(71, 72, 73, 74)}; return float4(x, 74); }
(the only difference with my previous email is the pair of braces around the initializer)
So it seems that the condition you want to discriminate on is whether the initializer is specified as a list (in which case you have to fully unpack and match in order) or as a single object (in which case you treat it as an assignment). I guess, at least.
No, I think that the original purpose of doing an assignment when "v->initializer.args_count == 1" is to cover the cases for when numeric variables are initialized with a single numerical constant.
I correct myself, you were right in that that was the condition that was tested here.
Regarding the compilation of that shader, yes, I hope it is acceptable to be permissive for those cases for now.
Maybe this can be treated when we implement the "flattening" of initializers: --- float4 main() : SV_TARGET { float4 aa = {{1, 2}, 3, {{{4}}}} return aa } ---
For instance, this shader:
float4 main() : sv_target { float3 aa = 3; float4 bb = {aa.x, aa.y, aa.z, 4.0}; return bb; }
Retrieves (3.0, 3.0, 3.0, 4.0) with the native compiler.
If I am not mistaken, initializers like float4(1 ,2 ,3 ,4) are treated the same as {1, 2, 3, 4} in the code.
I was mistaken, no, they aren't, obviously, sorry.
Still, I will add these tests in v3.
Hello,
January 12, 2022 1:35 PM, "Giovanni Mascellani" gmascellani@codeweavers.com wrote:
Hi,
this patch causes a regression on this shader:
float4 main() : SV_TARGET { float3 x = float4(71, 72, 73, 74); return float4(x, 74); }
This is accepted by native compiler and is works before your patch, but is broken after.
(see below)
On 10/01/22 20:33, Francisco Casas wrote:
@@ -1495,23 +1541,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t vkd3d_free(v); continue; }
- if (v->initializer.args_count > 1)
- if (type->type <= HLSL_CLASS_LAST_NUMERIC && size == 1)
{
- hlsl_fixme(ctx, &v->loc, "Complex initializer.");
- free_parse_initializer(&v->initializer);
- vkd3d_free(v);
- continue;
- struct hlsl_ir_load *load = hlsl_new_var_load(ctx, var, var->loc);
- list_add_tail(v->initializer.instrs, &load->node.entry);
- add_assignment(ctx, v->initializer.instrs, &load->node, ASSIGN_OP_ASSIGN,
v->initializer.args[0]); }
In other words, it seems that here you have to test for "v->initializer.args_count == 1", not "size == 1". But do your own research.
Yes, it is exactly that. Good catch!
Also, consider adding something similar to my example above to the tests (in the spirit of "everything that was gotten wrong at least once is a good candidate for a test").
Will do.
Thanks, Giovanni.
On 1/10/22 13:33, Francisco Casas wrote:
Signed-off-by: Francisco Casas fcasas@codeweavers.com
Makefile.am | 1 - libs/vkd3d-shader/hlsl.y | 72 ++++++++++++++++++++++++++++++++++------ 2 files changed, 61 insertions(+), 12 deletions(-)
diff --git a/Makefile.am b/Makefile.am index d15e50c3..1086d028 100644 --- a/Makefile.am +++ b/Makefile.am @@ -298,7 +298,6 @@ XFAIL_TESTS = \ tests/hlsl-initializer-flattening.shader_test \ tests/hlsl-initializer-invalid-n-args.shader_test \ tests/hlsl-initializer-local-array.shader_test \
- tests/hlsl-initializer-numeric.shader_test \ tests/hlsl-initializer-static-array.shader_test \ tests/hlsl-initializer-struct.shader_test \ tests/hlsl-bool-cast.shader_test \
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 988e0743..a89e4432 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -1278,6 +1278,54 @@ static bool add_increment(struct hlsl_ctx *ctx, struct list *instrs, bool decrem return true; }
+static void numeric_var_initializer(struct hlsl_ctx *ctx, struct hlsl_ir_var *var,
struct parse_variable_def *v, unsigned int reg_offset, struct hlsl_type *type,unsigned int *initializer_i, struct list *instrs)
I know this follows the naming convention of struct_var_initializer(), but as long as we're introducing new code, I'd rather give the function a name that has a verb in it.
+{
- unsigned int writemask_offset = 0;
- unsigned int components_read = 0;
- if (type->type == HLSL_CLASS_MATRIX)
hlsl_fixme(ctx, &var->loc, "Matrix initializer\n");
hlsl_fixme and hlsl_error messages should omit the trailing newline, and also be punctuated.
- while (components_read < hlsl_type_component_count(type))
- {
struct hlsl_ir_store *store;struct hlsl_ir_constant *c;struct hlsl_ir_node *node;unsigned int width;assert(*initializer_i < v->initializer.args_count);node = v->initializer.args[*initializer_i];*initializer_i += 1;
"initializer_i" makes it hard for me to understand what this variable is doing. How about "initializer_offset"?
Better yet, what I'd propose is that instead of passing the whole parse_variable_def or even parse_initializer, just pass "&v->initializer[initializer_offset]" to this function.
width = hlsl_type_component_count(node->data_type);components_read += width;if (components_read > hlsl_type_component_count(type)){hlsl_error(ctx, &node->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,"Initializer argument has spare components.\n");return;}
Isn't this already checked in declare_vars()?
if (!(node = add_implicit_conversion(ctx, instrs, node,hlsl_get_vector_type(ctx, type->base_type, width), &node->loc)))return;if (!(c = hlsl_new_uint_constant(ctx, reg_offset, node->loc)))return;list_add_tail(instrs, &c->node.entry);if (!(store = hlsl_new_store(ctx, var, &c->node, node,((1u << width) - 1) << writemask_offset, node->loc)))return;list_add_tail(instrs, &store->node.entry);writemask_offset += width;- }
+}
It's not quite a problem in this patch, since it doesn't implement recursion yet, but this won't work with the following shader:
---
uniform float4 u;
float4 main() : sv_target { float4 x[2] = {1, 2, u, 3, 4}; return x[1]; }
---
Note that "float4 u" can also be e.g. an array or struct, and native will still happily allow it.
This suggests to me that what we should probably do is actually "flatten" the initializer beforehand. That is, generate load and swizzle instructions such that every initializer argument is a scalar. E.g. the above would become {1, 2, u.x, u.y, u.z, u.w, 3, 4}. This generates a lot of unnecessary scalar ops, but I think we want a vectorization pass anyway.
Note that the same applies to constructors. E.g. the following is invalid:
uniform struct {float2 x, y;} u; float4 f = u;
but the following is valid:
uniform struct {float2 x, y;} u; float4 f = float4(u);
I think we should add a helper to "flatten" a parse_initializer, and then use it both in constructors and in complex initializers.
One of the nice effects of this is that initializer_size() goes away, and a lot of the size checking introduced by this patch series does too.
Hi,
Il 13/01/22 19:57, Zebediah Figura (she/her) ha scritto:
This suggests to me that what we should probably do is actually "flatten" the initializer beforehand. That is, generate load and swizzle instructions such that every initializer argument is a scalar. E.g. the above would become {1, 2, u.x, u.y, u.z, u.w, 3, 4}. This generates a lot of unnecessary scalar ops, but I think we want a vectorization pass anyway.
Fully agreed. Initially I thought this could arrive later, but I'm convincing myself that if we (meaning, in this case, Francisco :-P ) bite the bullet and immediately write the flattening, it becomes easier to check the rest and we have cleaner code.
As I already said, there are cases that are treated a bit differently, like "float3 x = float4(1, 2, 3, 4)", in which it is sensible to fall back to a simple assignment.
Giovanni.
On Fri, Jan 14, 2022 at 9:27 AM Giovanni Mascellani gmascellani@codeweavers.com wrote:
Hi,
Il 13/01/22 19:57, Zebediah Figura (she/her) ha scritto:
This suggests to me that what we should probably do is actually "flatten" the initializer beforehand. That is, generate load and swizzle instructions such that every initializer argument is a scalar. E.g. the above would become {1, 2, u.x, u.y, u.z, u.w, 3, 4}. This generates a lot of unnecessary scalar ops, but I think we want a vectorization pass anyway.
Fully agreed. Initially I thought this could arrive later, but I'm convincing myself that if we (meaning, in this case, Francisco :-P ) bite the bullet and immediately write the flattening, it becomes easier to check the rest and we have cleaner code.
Yes, same for me. Going flat (tm) should make everything nicer (except the resulting IR, but for that we have different tools).
As I already said, there are cases that are treated a bit differently, like "float3 x = float4(1, 2, 3, 4)", in which it is sensible to fall back to a simple assignment.
Right, it shouldn't be too complicated to special case those. This separate path can come after the generic / complex one, it's not like we're currently handling those initializers correctly anyway.
-
Francisco Casas -
Giovanni Mascellani -
Matteo Bruni -
Zebediah Figura (she/her)