From: Zebediah Figura Subject: Re: [PATCH 4/5] d3dcompiler: Calculate the register size of types. Message-Id: <6fd2f42c-bafd-39fd-74b3-99c47ffe7d64@codeweavers.com> Date: Wed, 1 Apr 2020 14:48:26 -0500 In-Reply-To: References: <20200330025320.564142-1-z.figura12@gmail.com> <20200330025320.564142-4-z.figura12@gmail.com> On 4/1/20 2:33 PM, Matteo Bruni wrote: > On Wed, Apr 1, 2020 at 9:00 PM Zebediah Figura wrote: >> >> 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...) > > Sure, using struct hlsl_type throughout the stack is not a problem. > hlsl_type is a bit of a bad name regardless. > The point about "it works for widl" isn't impressing me much though :D > > WRT DXBC offsets, you could have a struct bwriter_type (or other name) > that points to the relevant hlsl_type and has the additional stuff you > need for generating bytecode. It should be a bit nicer than sticking > everything in hlsl_type and hopefully not as painful as fully > duplicating everything (which I agree isn't helpful). It depends how > much bwriter-specific stuff you're going to need. Sure, that's definitely better than doing a whole translation. I'm mostly inclined to say it's not so much prettier that it's worth the extra allocation, but that's just me. I think the only thing I needed the bwriter layer to store is the type offset (including for sm4 reflection). > >> 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... > > Yeah, I guess storing the size in there is okay. I'll see how my body > will react to what you have in store next :D > >>> >>>> @@ -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. > > I don't think it matters a lot if it's stored in a separate field or > together with the other modifiers. This is a bit of a special case > regardless, in that you need to override the existing majority (rather > than complaining that the other majority is currently set) in some > cases. > Certainly fine to change things if it helps though. > > And yeah, tests do have room for improvement... > Mostly the trick is that a subsequent declaration "typedef column_major row_matrix_t col_matrix_t" throws up an error. Anyway, I'll add some extra tests.