From: Zebediah Figura Subject: Re: [PATCH 4/5] d3dcompiler: Calculate the register size of types. Message-Id: Date: Wed, 1 Apr 2020 13:52:13 -0500 In-Reply-To: References: <20200330025320.564142-1-z.figura12@gmail.com> <20200330025320.564142-4-z.figura12@gmail.com> On 4/1/20 1:34 PM, Matteo Bruni wrote: > On Mon, Mar 30, 2020 at 4:54 AM Zebediah Figura wrote: >> >> From: Zebediah Figura >> >> Signed-off-by: Zebediah Figura >> --- >> dlls/d3dcompiler_43/d3dcompiler_private.h | 3 +++ >> dlls/d3dcompiler_43/hlsl.y | 23 +++++++++++++++++++++++ >> dlls/d3dcompiler_43/utils.c | 9 +++++++++ >> 3 files changed, 35 insertions(+) >> >> diff --git a/dlls/d3dcompiler_43/d3dcompiler_private.h b/dlls/d3dcompiler_43/d3dcompiler_private.h >> index df45a7082fe..4fdb464a4ef 100644 >> --- a/dlls/d3dcompiler_43/d3dcompiler_private.h >> +++ b/dlls/d3dcompiler_43/d3dcompiler_private.h >> @@ -614,6 +614,7 @@ struct hlsl_type >> unsigned int modifiers; >> unsigned int dimx; >> unsigned int dimy; >> + unsigned int reg_size; >> union >> { >> struct list *elements; > > I'm not too thrilled to have the size stored in struct hlsl_type. I > guess it would be very awkward otherwise though... If you think this is bad, wait until you see what I have next :D (Namely, I have patches to access the hlsl_type structure from the "bwriter" layer, and also to store type offsets in the DXBC blob. Because otherwise writing structure types involves a lot of pointless duplication, both in terms of HLSL code and generated DXBC, and honestly widl does the same thing and it works. If it helps, we could rename it to something other than hlsl_type...) I guess the alternative here is to have something like "unsigned int hlsl_type_get_reg_size(const struct hlsl_type *type)". Not even that ugly, but obviously means more overhead, and I imagine avoiding that overhead is a worthwhile goal for a compiler... > >> @@ -632,6 +633,7 @@ struct hlsl_struct_field >> const char *name; >> const char *semantic; >> DWORD modifiers; >> + unsigned int reg_offset; >> }; >> >> struct source_location >> @@ -1083,6 +1085,7 @@ struct hlsl_type *new_hlsl_type(const char *name, enum hlsl_type_class type_clas >> struct hlsl_type *new_array_type(struct hlsl_type *basic_type, unsigned int array_size) DECLSPEC_HIDDEN; >> struct hlsl_type *clone_hlsl_type(struct hlsl_type *old) DECLSPEC_HIDDEN; >> struct hlsl_type *get_type(struct hlsl_scope *scope, const char *name, BOOL recursive) DECLSPEC_HIDDEN; >> +BOOL is_row_major(const struct hlsl_type *type) DECLSPEC_HIDDEN; >> BOOL find_function(const char *name) DECLSPEC_HIDDEN; >> unsigned int components_count_type(struct hlsl_type *type) DECLSPEC_HIDDEN; >> BOOL compare_hlsl_types(const struct hlsl_type *t1, const struct hlsl_type *t2) DECLSPEC_HIDDEN; >> diff --git a/dlls/d3dcompiler_43/hlsl.y b/dlls/d3dcompiler_43/hlsl.y >> index 312949c5840..d6c64edcace 100644 >> --- a/dlls/d3dcompiler_43/hlsl.y >> +++ b/dlls/d3dcompiler_43/hlsl.y >> @@ -717,6 +717,15 @@ static BOOL add_struct_field(struct list *fields, struct hlsl_struct_field *fiel >> return TRUE; >> } >> >> +BOOL is_row_major(const struct hlsl_type *type) >> +{ >> + if (type->modifiers & HLSL_MODIFIER_ROW_MAJOR) >> + return TRUE; >> + if (type->modifiers & HLSL_MODIFIER_COLUMN_MAJOR) >> + return FALSE; >> + return hlsl_ctx.matrix_majority == HLSL_ROW_MAJOR; >> +} >> + >> static struct hlsl_type *apply_type_modifiers(struct hlsl_type *type, DWORD *modifiers, struct source_location loc) >> { >> struct hlsl_type *new_type; >> @@ -729,6 +738,9 @@ static struct hlsl_type *apply_type_modifiers(struct hlsl_type *type, DWORD *mod >> >> new_type->modifiers = add_modifiers(new_type->modifiers, *modifiers, loc); >> *modifiers &= ~HLSL_TYPE_MODIFIERS_MASK; >> + >> + if (new_type->type == HLSL_CLASS_MATRIX) >> + new_type->reg_size = is_row_major(new_type) ? new_type->dimy : new_type->dimx; >> return new_type; >> } > > I think we should have one of the "majority" modifiers always stored > in the hlsl_type for matrices, since it's actually a significant > distinction. Notice that the default matrix majority can be changed > over the course of a shader via a #pragma. > The reason I don't particularly want to do this is constructions like: typedef float3x3 matrix_t; typedef row_major matrix_t row_matrix_t; I didn't realize that the default majority could change, though. Maybe we need to store the default majority as a separate field in hlsl_type.