Re: [PATCH vkd3d 3/6] vkd3d-shader: Write SM1 constant definitions.

[Zebediah Figura (she/her)]

On 4/26/21 7:48 AM, Matteo Bruni wrote:
> On Wed, Apr 21, 2021 at 6:30 AM Zebediah Figura  wrote:
> > Signed-off-by: Zebediah Figura
> >
> > include/vkd3d_d3d9types.h        |  41 +++++++++
> >   libs/vkd3d-shader/hlsl_codegen.c | 137 +++++++++++++++++++++++++++----
> >   2 files changed, 164 insertions(+), 14 deletions(-)
> >
> > diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> > index 0fd1f6bd..21ebcfc2 100644
> > --- a/libs/vkd3d-shader/hlsl_codegen.c
> > +++ b/libs/vkd3d-shader/hlsl_codegen.c
> > @@ -804,7 +804,19 @@ static void allocate_temp_registers_recurse(struct list *instrs, struct liveness
> >       }
> >   }
> >
> > -static void allocate_const_registers_recurse(struct list *instrs, struct liveness *liveness)
> > +struct vec4
> > +{
> > - float f[4];
> >
> > +};
> > -
> >
> > +struct constant_defs
> > +{
> > - struct vec4 *values;
> > - size_t count, size;
> >
> > +};
> > -
> >
> > +static void allocate_const_registers_recurse(struct hlsl_ctx *ctx, struct list *instrs, struct liveness *liveness,
> > -
> >    struct constant_defs *defs)
> > {
> >     struct hlsl_ir_node *instr;
> >
> > @@ -815,28 +827,83 @@ static void allocate_const_registers_recurse(struct list *instrs, struct livenes
> >               case HLSL_IR_CONSTANT:
> >               {
> >                   struct hlsl_ir_constant *constant = hlsl_ir_constant(instr);
> > -
> >            const struct hlsl_type *type = instr->data_type;
> > -
> >            unsigned int reg_size = type->reg_size, x, y, i, writemask;
> >
> > -
> >            if (instr->data_type->reg_size > 1)
> > -
> >                constant->reg = allocate_range(liveness, 1, UINT_MAX, instr->data_type->reg_size);
> > -
> >            if (reg_size > 1)
> > -
> >                constant->reg = allocate_range(liveness, 1, UINT_MAX, reg_size);
> >             else
> > -
> >                constant->reg = allocate_register(liveness, 1, UINT_MAX, instr->data_type->dimx);
> > -
> >            TRACE("Allocated constant @%u to %s.\n", instr->index,
> > -
> >                    debug_register('c', constant->reg, instr->data_type));
> > -
> >                constant->reg = allocate_register(liveness, 1, UINT_MAX, type->dimx);
> > -
> >            TRACE("Allocated constant @%u to %s.\n", instr->index, debug_register('c', constant->reg, type));
> > -
> > -
> >            if (!vkd3d_array_reserve((void **)&defs->values, &defs->size,
> > -
> >                    constant->reg.id + reg_size, sizeof(*defs->values)))
> > -
> >            {
> > -
> >                ctx->failed = true;
> > -
> >                return;
> > -
> >            }
> > -
> >            defs->count = constant->reg.id + reg_size;
>
> Maybe it's overkill, but I'd prefer if this was a max(defs->count,
> ...) so that it keeps working if allocate_range() /
> allocate_register() become smarter at some point in the future.
> > -
> > -
> >            assert(type->type <= HLSL_CLASS_LAST_NUMERIC);
> > -
> > -
> >            if (!(writemask = constant->reg.writemask))
> > -
> >                writemask = (1 << type->dimx) - 1;
>
> Usual nitpick, here and elsewhere: 1u as the shift operand.
> > @@ -1208,6 +1316,7 @@ static int write_sm1_shader(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *
> >
> > int hlsl_emit_dxbc(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry_func, struct vkd3d_shader_code *out)
> >   {
> > -
> >
> > struct constant_defs constant_defs;
> >  struct hlsl_ir_var *var;
> >
> > list_move_head(entry_func->body, &ctx->static_initializers);
> >
> > @@ -1249,13 +1358,13 @@ int hlsl_emit_dxbc(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry_fun
> >   allocate_temp_registers(entry_func);
> >   if (ctx->profile->major_version < 4)
> > -
> >    allocate_const_registers(ctx, entry_func);
> > -
> >    constant_defs = allocate_const_registers(ctx, entry_func);
> >
> > if (ctx->failed)
> >     return VKD3D_ERROR_INVALID_SHADER;
> >
> > if (ctx->profile->major_version < 4)
> > -
> >    return write_sm1_shader(ctx, entry_func, out);
> > -
> >    return write_sm1_shader(ctx, entry_func, &constant_defs, out);
> > else
> >     return VKD3D_ERROR_NOT_IMPLEMENTED;
> > }
>
> I guess it's a matter of taste but any particular reason why
> constant_defs isn't part of the context?

Probably just "it doesn't need to be", but I don't object to putting it 
there.