March 7, 2022
5:33 p.m.
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