On 4/28/22 16:30, Francisco Casas wrote:
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.
Right, the assert works to clarify.
I'm not sure whether "float a[2][]" should be a syntax error, though. As elsewhere, it'd be more helpful to explain why it's wrong, and probably also to avoid failing compilation immediately.
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.
Well, it at least can't be done until we have type information, which means it'd need to be deferred until declare_vars and gen_struct_fields(). That sounds like conceptually the right place for it, to me.
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.");
Here ^
- 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.
It's not something that this patch necessarily needs to handle, even with a FIXME, but it should inform how the code is structured, which is why I bring it up.