[PATCH 4/5] d3dcompiler: Calculate the register size of types.

[Zebediah Figura]

On 4/1/20 2:33 PM, Matteo Bruni wrote:
> On Wed, Apr 1, 2020 at 9:00 PM Zebediah Figura <zfigura at codeweavers.com> wrote:
>>
>> On 4/1/20 1:34 PM, Matteo Bruni wrote:
>>> On Mon, Mar 30, 2020 at 4:54 AM Zebediah Figura <z.figura12 at gmail.com> wrote:
>>>>
>>>> From: Zebediah Figura <zfigura at codeweavers.com>
>>>>
>>>> Signed-off-by: Zebediah Figura <zfigura at codeweavers.com>
>>>> ---
>>>>   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.