Re: [PATCH vkd3d v3 2/8] vkd3d-shader/hlsl: Add default writemask for matrix stores.

[Zebediah Figura]

On 3/7/22 09:09, Francisco Casas wrote:
> Hello,
> 
> March 4, 2022 10:01 PM, "Zebediah Figura (she/her)" <zfigura(a)codeweavers.com> wrote:
> 
>> On 3/2/22 12:31, Francisco Casas wrote:
>>
>>> Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com>
>>> ---
>>> v3:
>>> - This patch is new.
>>> - While we don't use the default writemask for matrices (yet),
>>> I think it is good for consistency, since we are getting rid
>>> of the type_is_single_reg() function.
>>> Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com>
>>> ---
>>> libs/vkd3d-shader/hlsl.c | 12 ++++++++----
>>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>> diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
>>> index 00a374b4..5152aec4 100644
>>> --- a/libs/vkd3d-shader/hlsl.c
>>> +++ b/libs/vkd3d-shader/hlsl.c
>>> @@ -527,9 +527,13 @@ struct hlsl_ir_var *hlsl_new_synthetic_var(struct hlsl_ctx *ctx, const char
>>> *nam
>>> return var;
>>> }
>>>> -static bool type_is_single_reg(const struct hlsl_type *type)
>>> +static unsigned int type_default_writemask(const struct hlsl_type *type)
>>> - return type->type == HLSL_CLASS_SCALAR || type->type == HLSL_CLASS_VECTOR;
>>> + if (type->type == HLSL_CLASS_MATRIX && hlsl_type_is_row_major(type))
>>> + return (1 << type->dimy) - 1;
>>
>> I don't understand the logic here. A nonzero writemask should encompass the whole type, which this
>> does not. Why do we need this?
>>
> 
> Well, I am not totally sure this patch is actually necessary, I just
> thought it could be a good default behavior since we are getting rid of
> hlsl_is_single_reg(), but maybe I am overthinking...

FWIW, I don't know that that instance of type_is_single_reg() 
necessarily needs to go away. Although given 1/8, we could certainly say 
that a zero writemask always means the whole variable, and let 
hlsl_reg_from_deref() deal with that even for scalar and vector types.

> 
> As you know, if we have a HLSL_IR_STORE with a whole matrix in the rhs,
> we have to split it in up to 4 stores, each one with a writemask that
> matches the result of this type_default_writemask() here.

Well, yeah, but we should be using the result type in that case, i.e. 
vectors rather than matrices.

> 
> The patch from Giovanni that implements this split_matrix_copies()
> pass (which is many patches ahead), makes sure to do it with only vectors
> in the rhs.
> 
> But currently, if I am not mistaken (I could be), a HLSL_IR_STORE with a
> matrix in the rhs is producing a single MOV instruction for the first
> register that the matrix uses. Which maybe could be problematic in sm4
> with matrices with major size =1 and minor size <4.

Currently it's not producing anything, actually; we have a FIXME for 
that case in write_sm[14]_instructions().

> 
> I added this default behavior thinking on those cases (if they could actually
> happen), and maybe because I thought it could be useful in the future if for
> some reason we wanted to implement split_matrix_copies() in other way, by
> directly copying the store instructions.
> 
> But well, maybe I'll just do
> ---
> if (!writemask && (type->type == HLSL_CLASS_SCALAR || type->type == HLSL_CLASS_VECTOR))
>      writemask = (1 << rhs->data_type->dimx) - 1;
> ---
> or remove the patch altogether in the next version of the batch.
> 
> 
> Best regards,
> Francisco

[PATCH vkd3d v3 2/8] vkd3d-shader/hlsl: Add default writemask for matrix stores.

[Matteo Bruni]

On Mon, Mar 7, 2022 at 6:33 PM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
>
> On 3/7/22 09:09, Francisco Casas wrote:
> > Hello,
> >
> > March 4, 2022 10:01 PM, "Zebediah Figura (she/her)" <zfigura(a)codeweavers.com> wrote:
> >
> >> On 3/2/22 12:31, Francisco Casas wrote:
> >>
> >>> Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com>
> >>> ---
> >>> v3:
> >>> - This patch is new.
> >>> - While we don't use the default writemask for matrices (yet),
> >>> I think it is good for consistency, since we are getting rid
> >>> of the type_is_single_reg() function.
> >>> Signed-off-by: Francisco Casas <fcasas(a)codeweavers.com>
> >>> ---
> >>> libs/vkd3d-shader/hlsl.c | 12 ++++++++----
> >>> 1 file changed, 8 insertions(+), 4 deletions(-)
> >>> diff --git a/libs/vkd3d-shader/hlsl.c b/libs/vkd3d-shader/hlsl.c
> >>> index 00a374b4..5152aec4 100644
> >>> --- a/libs/vkd3d-shader/hlsl.c
> >>> +++ b/libs/vkd3d-shader/hlsl.c
> >>> @@ -527,9 +527,13 @@ struct hlsl_ir_var *hlsl_new_synthetic_var(struct hlsl_ctx *ctx, const char
> >>> *nam
> >>> return var;
> >>> }
> >>>> -static bool type_is_single_reg(const struct hlsl_type *type)
> >>> +static unsigned int type_default_writemask(const struct hlsl_type *type)
> >>> - return type->type == HLSL_CLASS_SCALAR || type->type == HLSL_CLASS_VECTOR;
> >>> + if (type->type == HLSL_CLASS_MATRIX && hlsl_type_is_row_major(type))
> >>> + return (1 << type->dimy) - 1;
> >>
> >> I don't understand the logic here. A nonzero writemask should encompass the whole type, which this
> >> does not. Why do we need this?
> >>
> >
> > Well, I am not totally sure this patch is actually necessary, I just
> > thought it could be a good default behavior since we are getting rid of
> > hlsl_is_single_reg(), but maybe I am overthinking...
>
> FWIW, I don't know that that instance of type_is_single_reg()
> necessarily needs to go away. Although given 1/8, we could certainly say
> that a zero writemask always means the whole variable, and let
> hlsl_reg_from_deref() deal with that even for scalar and vector types.

I'd move in that direction too (shorter term, at least).