April 28, 2022 4:22 PM, "Zebediah Figura" <zfigura@codeweavers.com> wrote:

On 4/28/22 14:45, Francisco Casas wrote:

diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y
index 905dbfc5..e7fe74d8 100644
--- a/libs/vkd3d-shader/hlsl.y
+++ b/libs/vkd3d-shader/hlsl.y
@@ -1606,7 +1606,27 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type
*basic_t
type = basic_type;
for (i = 0; i < v->arrays.count; ++i)
+ {
+ if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
+ {
+ unsigned int size = initializer_size(&v->initializer);
+ unsigned int elem_components = hlsl_type_component_count(type);
+
+ assert(v->initializer.args_count);
+
+ v->arrays.sizes[i] = (size + elem_components - 1)/elem_components;
+
+ if (size % elem_components != 0)
+ {
+ hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
+ "Cannot initialize implicit array with %u components, expected a multiple of %u.",
+ size, elem_components);
+ free_parse_initializer(&v->initializer);
+ v->initializer.args_count = 0;
+ }

This doesn't seem like it'll do the right thing for implicit sizes on an inner array, especially
considering that the right thing is probably to fail compilation.

It also won't do the right thing for initializers without braces (viz. also fail compilation).

These cases are checked in the parse rules introduced in this patch.

Oh, I see, I wasn't actually correctly reading the parser rule. That makes sense.

On the other hand, handling IMPLICIT inside of the loop, without any checks, was one of the things
that made me think "we're not handling inner arrays correctly", so arguably something deserves
changing here :-)

Well, the structure of the parsing rule ensures that only the most external array could have
IMPLICIT side. This is related to the
assert(i == v->arrays.count - 1);
suggested by Giovanni.

I don't see any handling for braceless initializers in this patch, though; am I missing something?

The error is added to the "variable_def:" rule.
But then again, maybe it is not so clear that the parse rule ensures that only the most external array
can have IMPLICIT size. I could add the same assertion here too.

By the way, I am not sure if you changed your stance on checking the implicit size arrays at the parse
level. I would prefer to keep it there... if I find a way of also handling these unbounded resource
arrays nicely.

I think we need tests for all of these corner cases, and also a test for missing initializers.

Okay, adding them.
+ }
type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]);
+ }
vkd3d_free(v->arrays.sizes);
if (type->type != HLSL_CLASS_MATRIX)
@@ -2464,6 +2484,7 @@ static bool add_method_call(struct hlsl_ctx *ctx, struct list *instrs, struct
hl
%token <name> TYPE_IDENTIFIER
%type <arrays> arrays
+%type <arrays> implicit_arrays
%type <assign_op> assign_op
@@ -3108,7 +3129,7 @@ variables_def:
}
variable_decl:
- any_identifier arrays colon_attribute
+ any_identifier implicit_arrays colon_attribute
{
$$ = hlsl_alloc(ctx, sizeof(*$$));
$$->loc = @1;
@@ -3137,6 +3158,15 @@ state_block:
variable_def:
variable_decl
+ {
+ if ($$->arrays.sizes && $$->arrays.sizes[$$->arrays.count - 1] ==
HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
+ {
+ hlsl_error(ctx, &@1, VKD3D_SHADER_ERROR_HLSL_MISSING_INITIALIZER,
+ "Implicit array requires initializer.");
+ free_parse_variable_def($$);
+ YYABORT;
+ }
+ }

This won't work for unbounded resource arrays, which can be declared with an empty pair of brackets
(and no initializer).

I see, I didn't knew about those.
It also aborts compilation somewhat unnecessarily.

AFAIK (except for unbounded resource arrays) the native compiler doesn't allow implicit size arrays
without initializer, so aborting seemed logical.

Failing compilation is fine, but in general I think we want to avoid aborting if we can help it. We
should only abort if we really can't continue parsing, e.g. if we encounter a syntax error. That
way, if there are multiple errors, the (HLSL) programmer can deal with them all at once.

(As a side note, perhaps we should use zero instead of UINT_MAX. Unbounded resources in shader

model 5.1 are encoded as zero in the reflection data, whereas declaring a resource array as e.g.
"Texture2D t[0xffffffff]" yields 0xffffffff instead, although the shader bytecode is identical.)

As a special extra, this code is apparently valid, and I think deserves to be a test case:

struct apple
{
Texture2D t[];
};

Hmm, this is interesting. Maybe it is intended for input semantics?
I will investigate.

Not for input semantics, but rather for bound resources. Shader model 5.0 allows declaring arrays
of textures, and shader model 5.1 (and Direct3D 12) allows declaring "unbounded" arrays, as e.g.
"Texture2D t[]" or "Texture2D t[0]".

Normally textures aren't declared as part of a struct, but I was curious to see if it was legal,
and it turns out it is. (Although the reflection type information that's generated isn't quite
correct.)

Hmm, I will see where I can add a FIXME for those cases for now then.