Re: [PATCH vkd3d v3 01/12] vkd3d-shader/hlsl: Add support for sm4 instruction modifiers.

[Matteo Bruni]

On Fri, Dec 17, 2021 at 10:21 PM Zebediah Figura (she/her)
 wrote:
> On 12/17/21 13:12, Francisco Casas wrote:
> > Signed-off-by: Francisco Casas
> >
> > libs/vkd3d-shader/hlsl_sm4.c | 54 ++++++++++++++++++++++++++++++++++++
> >   1 file changed, 54 insertions(+)
> >
> > diff --git a/libs/vkd3d-shader/hlsl_sm4.c b/libs/vkd3d-shader/hlsl_sm4.c
> > index 2458018f..07bbd376 100644
> > --- a/libs/vkd3d-shader/hlsl_sm4.c
> > +++ b/libs/vkd3d-shader/hlsl_sm4.c
> > @@ -727,6 +727,46 @@ static enum vkd3d_sm4_resource_type sm4_resource_dimension(const struct hlsl_typ
> >       }
> >   }
> >
> > +struct sm4_instruction_modifier
> > +{
> > - enum vkd3d_sm4_instruction_modifier type;
> > -
> > - union
> > - {
> > -
> >    struct
> > -
> >    {
> > -
> >        int u,v,w;
> > -
> >    } aoffimmi;
> > - };
> >
> > +};
> > -
> >
> > +static uint32_t sm4_encode_instruction_modifier(struct sm4_instruction_modifier imod)
>
> This should be a const pointer instead, I think.
> > +{
> > - uint32_t word = 0;
> > -
> > - word |= VKD3D_SM4_MODIFIER_MASK & imod.type;
> > -
> > - switch (imod.type)
> > - {
> > -
> >    case VKD3D_SM4_MODIFIER_AOFFIMMI:
> > -
> >        assert(-8 <= imod.aoffimmi.u && imod.aoffimmi.u <= 7);
> > -
> >        assert(-8 <= imod.aoffimmi.v && imod.aoffimmi.v <= 7);
> > -
> >        assert(-8 <= imod.aoffimmi.w && imod.aoffimmi.w <= 7);
> > -
> >        word |= ((uint32_t)imod.aoffimmi.u & 0xf) << VKD3D_SM4_AOFFIMMI_U_SHIFT;
> > -
> >        word |= ((uint32_t)imod.aoffimmi.v & 0xf) << VKD3D_SM4_AOFFIMMI_V_SHIFT;
> > -
> >        word |= ((uint32_t)imod.aoffimmi.w & 0xf) << VKD3D_SM4_AOFFIMMI_W_SHIFT;
> > -
> >    break;
> > -
> > -
> >    default:
> > -
> >        FIXME("Unhandled instruction modifier %#x.\n", imod.type);
> > -
> >        return 0;
> > -
> >    break;
> > - }
> > -
> > - return word;
> >
> > +}
> > -
> > - struct sm4_register
> > {
> >     enum vkd3d_sm4_register_type type;
> >
> > @@ -741,6 +781,9 @@ struct sm4_instruction
> >   {
> >       enum vkd3d_sm4_opcode opcode;
> > - struct sm4_instruction_modifier modifiers[1];
> > - unsigned int modifier_count;
> > -
> > struct
> > {
> >     struct sm4_register reg;
> >
> > @@ -913,6 +956,7 @@ static void write_sm4_instruction(struct vkd3d_bytecode_buffer *buffer, const st
> >       uint32_t token = instr->opcode;
> >       unsigned int size = 1, i, j;
> > - size += instr->modifier_count;
> >  for (i = 0; i < instr->dst_count; ++i)
> >      size += sm4_register_order(&instr->dsts[i].reg);
> >  for (i = 0; i < instr->src_count; ++i)
> >
> > @@ -920,8 +964,18 @@ static void write_sm4_instruction(struct vkd3d_bytecode_buffer *buffer, const st
> >       size += instr->idx_count;
> >   token |= (size << VKD3D_SM4_INSTRUCTION_LENGTH_SHIFT);
> > -
> > - token |= ((0 < instr->modifier_count) << 31);
> > -
>
> Ech, this is hard to read, how about a simpler:
>  if (instr->modifier_count > 0)
>      token |= VKD3D_SM4_INSTRUCTION_MODIFIER;

Or even

if (instr->modifier_count)
    token |= VKD3D_SM4_INSTRUCTION_MODIFIER;