Re: [PATCH vkd3d 2/6] vkd3d-shader/hlsl: Write SM4 store instructions.

[Matteo Bruni]

On Thu, Aug 26, 2021 at 6:23 PM Zebediah Figura (she/her)
 wrote:
> On 8/26/21 10:47 AM, Matteo Bruni wrote:
> > On Tue, Aug 24, 2021 at 8:18 AM Zebediah Figura  wrote:
> > > Signed-off-by: Zebediah Figura
> > >
> > > libs/vkd3d-shader/hlsl_sm4.c | 116 ++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 114 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c
> > > index 15927671..2e304c5a 100644
> > > --- a/libs/vkd3d-shader/hlsl_sm4.c
> > > +++ b/libs/vkd3d-shader/hlsl_sm4.c
> > > @@ -599,6 +600,64 @@ static unsigned int sm4_swizzle_type(enum vkd3d_sm4_register_type type)
> > >       }
> > >   }
> > >
> > > +static void sm4_register_from_deref(struct hlsl_ctx *ctx, struct sm4_register *reg,
> > > -
> > >    unsigned int *writemask, const struct hlsl_deref *deref, const struct hlsl_type *data_type)
> > >
> > > +{
> > > - const struct hlsl_ir_var *var = deref->var;
> > > -
> > > - if (var->is_output_semantic)
> > > - {
> > > -
> > >    bool has_idx;
> > > -
> > > -
> > >    if (hlsl_sm4_register_from_semantic(ctx, &var->semantic, true, &reg->type, &has_idx))
> > > -
> > >    {
> > > -
> > >        if (has_idx)
> > > -
> > >        {
> > > -
> > >            reg->idx[0] = var->semantic.index;
> > > -
> > >            reg->idx_count = 1;
> > > -
> > >        }
> > > -
> > > -
> > >        if (reg->type == VKD3D_SM4_RT_DEPTHOUT)
> > > -
> > >            reg->dim = VKD3D_SM4_DIMENSION_SCALAR;
> > > -
> > >        else
> > > -
> > >            reg->dim = VKD3D_SM4_DIMENSION_VEC4;
> > > -
> > >        *writemask = (1u << data_type->dimx) - 1;
> > > -
> > >    }
> > > -
> > >    else
> > > -
> > >    {
> > > -
> > >        struct hlsl_reg hlsl_reg = hlsl_reg_from_deref(deref, data_type);
> > > -
> > > -
> > >        assert(hlsl_reg.allocated);
> > > -
> > >        reg->type = VKD3D_SM4_RT_OUTPUT;
> > > -
> > >        reg->dim = VKD3D_SM4_DIMENSION_VEC4;
> > > -
> > >        reg->idx[0] = hlsl_reg.id;
> > > -
> > >        reg->idx_count = 1;
> > > -
> > >        *writemask = hlsl_reg.writemask;
> > > -
> > >    }
> > > - }
> > > - else
> > > - {
> > > -
> > >    struct hlsl_reg hlsl_reg = hlsl_reg_from_deref(deref, data_type);
> > > -
> > > -
> > >    assert(hlsl_reg.allocated);
> > > -
> > >    reg->type = VKD3D_SM4_RT_TEMP;
> > > -
> > >    reg->dim = VKD3D_SM4_DIMENSION_VEC4;
> > > -
> > >    reg->idx[0] = hlsl_reg.id;
> > > -
> > >    reg->idx_count = 1;
> > > -
> > >    *writemask = hlsl_reg.writemask;
> > > - }
> > >
> > > +}
> > > -
> > >
> > > +static void sm4_register_from_node(struct sm4_register *reg, unsigned int *writemask, const struct hlsl_ir_node *instr)
> > > +{
> > > - assert(instr->reg.allocated);
> > > - reg->type = VKD3D_SM4_RT_TEMP;
> > > - reg->dim = VKD3D_SM4_DIMENSION_VEC4;
> > > - reg->idx[0] = instr->reg.id;
> > > - reg->idx_count = 1;
> > > - *writemask = instr->reg.writemask;
> > >
> > > +}
> > > - static uint32_t sm4_encode_register(const struct sm4_register *reg)
> > > {
> > >     return (reg->type << VKD3D_SM4_REGISTER_TYPE_SHIFT)
> >
> > I'm a bit annoyed by the fact that sm4_register_from_deref() here
> > takes into account only the subset of derefs / registers that matter
> > for the store instruction. OTOH this is "fixed" in the next patch and
> > there aren't clearly better alternatives, so I guess I'll bite the
> > bullet :)
>
> I considered doing it the other way, but I didn't really want to add
> dead code. Probably the right thing to do was add a FIXME...

Eh, it's fine. Annoyed isn't quite the word anyway, more like mildly
surprised / confused, and it's all settled with a more thorough look.