Hello,
May 6, 2022 4:38 AM, "Giovanni Mascellani" gmascellani@codeweavers.com wrote:
Hi,
thanks for reworking again your patches, unfortunately it seems to me that there are still points that can be enhanced.
No problem, thanks for having me patience.
Il 05/05/22 15:16, Giovanni Mascellani ha scritto:
diff --git a/libs/vkd3d-shader/hlsl.y b/libs/vkd3d-shader/hlsl.y index 44e4964f..9fe454d0 100644 --- a/libs/vkd3d-shader/hlsl.y +++ b/libs/vkd3d-shader/hlsl.y @@ -779,6 +779,7 @@ static struct list *gen_struct_fields(struct hlsl_ctx *ctx, return NULL; LIST_FOR_EACH_ENTRY_SAFE(v, v_next, fields, struct parse_variable_def, entry) {
- bool unbounded_res_array = false;
unsigned int i;
if (!(field = hlsl_alloc(ctx, sizeof(*field))))
@@ -789,7 +790,33 @@ static struct list *gen_struct_fields(struct hlsl_ctx *ctx,
field->type = type;
for (i = 0; i < v->arrays.count; ++i)
- {
- if (v->arrays.sizes[i] == HLSL_ARRAY_ELEMENTS_COUNT_IMPLICIT)
- {
- if (i < v->arrays.count - 1)
- {
- hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
- "Inner array size cannot be implicit.");
- }
- else if (type->type == HLSL_CLASS_OBJECT)
- {
- unbounded_res_array = true;
- }
- else
- {
- hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
- "Implicit size arrays not allowed in struct fields.");
- }
- }
field->type = hlsl_new_array_type(ctx, field->type, v->arrays.sizes[i]);
- }
- if (unbounded_res_array)
- {
- hlsl_fixme(ctx, &v->loc, "Unbounded resource arrays as struct fields.");
- free_parse_variable_def(v);
- vkd3d_free(field);
- continue;
- }
vkd3d_free(v->arrays.sizes); field->loc = v->loc; field->name = v->name;
Here we are in the context of a struct field, where is seems that any kind of unbounded array is forbidden, isn't it? I mean, it should be an hlsl_error(), not an hlsl_fixme(): this program fails to compile with native:
struct ss { Texture2D x[]; };
float4 main() : sv_target { struct ss arr;
return 0.0; }
As Zebediah pointed out in her reviews of my previous patches, in ps_5_1 and vs_5_1, arrays of textures are processed as a different datatype, and they are allowed to be implicit, and even be struct members. They are called "unbounded resource arrays".
For instance, your example actually compiles with ps_5_1.
So it was decided to drop those variables for now.
Once this is taken into account, it seems that any kind of implicit array is just forbidden inside a struct definition, so you can avoid different cases depending on the underlying type: if you see _COUNT_IMPLICIT inside gen_struct_field() you just hlsl_error() out (just like you do for typedefs, for instance). This would also agree with the native d3dcompiler's error message: "array dimensions of struct/class members must be explicit", though that's not necessarily a strong argument. Or an argument at all. Just a funny coincidence.
Well, as I mentioned that's not totally true in ps_5_1.
I could check that the profile is >= 5.1 in the conditionals.
@@ -1601,11 +1634,67 @@ static struct list *declare_vars(struct hlsl_ctx *ctx, struct hlsl_type *basic_t
LIST_FOR_EACH_ENTRY_SAFE(v, v_next, var_list, struct parse_variable_def, entry)
{
- bool unbounded_res_array = false;
unsigned int i;
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);
- if (i < v->arrays.count - 1)
- {
- hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
- "Inner array size cannot be implicit.");
- free_parse_initializer(&v->initializer);
- v->initializer.args_count = 0;
- }
- else if (elem_components == 0)
- {
- hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
- "Cannot declare an implicit size array of a size 0 type.");
- free_parse_initializer(&v->initializer);
- v->initializer.args_count = 0;
- }
- else if (size == 0)
- {
- if (type->type == HLSL_CLASS_OBJECT)
- {
- unbounded_res_array = 1;
- }
- else
- {
- hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_TYPE,
- "Implicit size arrays need to be initialized.");
- free_parse_initializer(&v->initializer);
- v->initializer.args_count = 0;
- }
- }
- else if (size % elem_components != 0)
- {
- hlsl_error(ctx, &v->loc, VKD3D_SHADER_ERROR_HLSL_WRONG_PARAMETER_COUNT,
- "Cannot initialize implicit size array with %u components, expected a multiple of %u.",
- size, elem_components);
- free_parse_initializer(&v->initializer);
- v->initializer.args_count = 0;
- }
- else
- {
- v->arrays.sizes[i] = size / elem_components;
- }
- }
type = hlsl_new_array_type(ctx, type, v->arrays.sizes[i]);
- }
- if (unbounded_res_array)
- {
- hlsl_fixme(ctx, &v->loc, "Unbounded resource arrays.");
- free_parse_variable_def(v);
- continue;
- }
vkd3d_free(v->arrays.sizes);
if (type->type != HLSL_CLASS_MATRIX)
Isn't the case "type->type == HLSL_CLASS_OBJECT" already covered by "elem_components == 0"? When type is of OBJECT class, then hlsl_type_component_count() always spits out zero (and logs an ERR(); arguably it should return an error condition which should be handled by the caller, but maybe this issue can be postponed).
I see. I have been pushing for hlsl_type_component_count() to return 1 with objects, so that initializers work correctly on structs that have object members, like in:
--- Texture2D tex;
struct foo { float2 aa; Texture2D bb[2]; float4 cc; };
float4 main() : sv_target { struct foo q = {10, 20, tex, tex, 30, 40, 50, 60};
return q.cc; } ---
This indeed seems necessary for the condition '(size == 0)' to not be redundant.
Instead of removing it, I suggest considering first a patch so that hlsl_type_component_count() returns 1 for objects. I will get to work on it.
BTW, notice that you can use "true" instead of "1" to assign to a boolean variable.
Agh! Sorry! It is just a habit.
Giovanni.