Re: [PATCH vkd3d 5/5] vkd3d-shader: Write SM1 store instructions.

[Zebediah Figura (she/her)]

On 5/14/21 4:41 AM, Matteo Bruni wrote:
> On Tue, May 11, 2021 at 6:36 AM Zebediah Figura  wrote:
> > Signed-off-by: Zebediah Figura
> >
> > include/vkd3d_d3d9types.h        |  22 ++++
> >  libs/vkd3d-shader/hlsl_codegen.c | 220 +++++++++++++++++++++++++++++++
> >  2 files changed, 242 insertions(+)
>
> Very nice. The generated bytecode looks a bit silly right now, because
> oftentimes we end up generating instructions like "mov r0, r0" but 1.
> it doesn't matter and 2. it shouldn't be hard to clean it up at some
> point.
> > diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> > index d4995110..0dfbfb9f 100644
> > --- a/libs/vkd3d-shader/hlsl_codegen.c
> > +++ b/libs/vkd3d-shader/hlsl_codegen.c
> > +static struct hlsl_reg hlsl_reg_from_deref(const struct hlsl_deref *deref, const struct hlsl_type *type)
> > +{
> > - struct hlsl_ir_node *offset_node = deref->offset.node;
> > - const struct hlsl_ir_var *var = deref->var;
> > - struct hlsl_reg ret = {0};
> > - unsigned int offset = 0;
> > -
> > - if (offset_node && offset_node->type != HLSL_IR_CONSTANT)
> > - {
> > -
> >    FIXME("Dereference with non-constant offset of type %s.\n", hlsl_node_type_to_string(offset_node->type));
> > -
> >    return ret;
> > - }
> > -
> > - ret = var->reg;
> > -
> > - ret.allocated = var->reg.allocated;
> > - ret.id = var->reg.id;
> > - if (offset_node)
> > -
> >    offset = hlsl_ir_constant(offset_node)->value.u[0];
>
> Maybe it makes sense to assert() that the offset is a HLSL_TYPE_UINT scalar?

Eh, yes, that's a good idea, will do.
> > @@ -1428,6 +1523,57 @@ static uint32_t sm1_encode_dst(D3DSHADER_PARAM_REGISTER_TYPE type,
> >      return (1u << 31) | sm1_encode_register_type(type) | modifier | (writemask << 16) | reg;
> >  }
> >
> > +static uint32_t sm1_encode_src(D3DSHADER_PARAM_REGISTER_TYPE type,
> > -
> >    D3DSHADER_PARAM_SRCMOD_TYPE modifier, unsigned int swizzle, uint32_t reg)
> >
> > +{
> > - return (1u << 31) | sm1_encode_register_type(type) | modifier | (swizzle << 16) | reg;
> >
> > +}
> > -
> >
> > +struct sm1_instruction
> > +{
> > - D3DSHADER_INSTRUCTION_OPCODE_TYPE opcode;
> > -
> > - struct
> > - {
> > -
> >    D3DSHADER_PARAM_REGISTER_TYPE type;
> > -
> >    D3DSHADER_PARAM_DSTMOD_TYPE mod;
> > -
> >    unsigned int writemask;
> > -
> >    uint32_t reg;
> > - } dst;
> > -
> > - struct
> > - {
> > -
> >    D3DSHADER_PARAM_REGISTER_TYPE type;
> > -
> >    D3DSHADER_PARAM_SRCMOD_TYPE mod;
> > -
> >    unsigned int swizzle;
> > -
> >    uint32_t reg;
> > - } srcs[2];
> > - unsigned int src_count;
> > -
> > - unsigned int has_dst;
> >
> > +};
>
> It might make sense to embrace this direction and e.g. assign a type
> to the src / dst register structs, so that you can pass a pointer to
> the struct to sm1_encode_(src,dst)() instead of each piece of register
> specification individually.

I think the only reason I didn't do this in the first place was because
of semantic declarations, but honestly it should be easy to just create
structures, if not sm1_instruction as a whole.

That, or this code was gradually transformed from a version that didn't
even have the sm1_instruction helper.
> We could even go a step further and have
> an array of struct sm1_instruction as a low-level IR, splitting the
> actual bytecode emission from the instruction generation.
>
> Not a must by any means, but do keep the possibility in mind: there is
> a non-zero chance that some transformation passes become much simpler
> in a lower level IR.

There are probably some optimization passes that do get easy at a very
low level, though I can't immediately of any that can't also be solved
at a higher level. Plus hlsl_ir could probably be made lower-level than
it is anyway. I dunno, compilers are hard...