Re: [PATCH vkd3d 1/5] vkd3d-shader/hlsl: Allocate textures for SM4.

[Zebediah Figura]

On 10/14/21 3:45 AM, Matteo Bruni wrote:
> On Tue, Oct 12, 2021 at 4:59 AM Zebediah Figura  wrote:
> > Signed-off-by: Zebediah Figura
> >
> > libs/vkd3d-shader/hlsl_codegen.c | 63 ++++++++++++++++++++++++++++++++
> >   1 file changed, 63 insertions(+)
> >
> > diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> > index 00fe76b2f..bf93a03df 100644
> > --- a/libs/vkd3d-shader/hlsl_codegen.c
> > +++ b/libs/vkd3d-shader/hlsl_codegen.c
> > @@ -1197,6 +1197,64 @@ static void allocate_buffers(struct hlsl_ctx *ctx)
> >       }
> >   }
> >
> > +static const struct hlsl_ir_var *get_reserved_texture(struct hlsl_ctx *ctx, uint32_t index)
> > +{
> > - const struct hlsl_ir_var *var;
> > -
> > - LIST_FOR_EACH_ENTRY(var, &ctx->extern_vars, const struct hlsl_ir_var, extern_entry)
> > - {
> > -
> >    if (var->has_resource_access && var->reg_reservation.type == 't' && var->reg_reservation.index == index)
> > -
> >        return var;
> > - }
> > - return NULL;
> >
> > +}
> > -
> >
> > +static void allocate_textures(struct hlsl_ctx *ctx)
> > +{
> > - struct hlsl_ir_var *var;
> > - uint32_t index = 0;
> > -
> > - LIST_FOR_EACH_ENTRY(var, &ctx->extern_vars, struct hlsl_ir_var, extern_entry)
> > - {
> > -
> >    if (!var->has_resource_access || var->data_type->type != HLSL_CLASS_OBJECT
> > -
> >            || var->data_type->base_type != HLSL_TYPE_TEXTURE)
> > -
> >        continue;
> > -
> > -
> >    if (var->reg_reservation.type == 't')
> > -
> >    {
> > -
> >        const struct hlsl_ir_var *reserved_texture = get_reserved_texture(ctx, var->reg_reservation.index);
> > -
> > -
> >        if (reserved_texture && reserved_texture != var)
> > -
> >        {
> > -
> >            hlsl_error(ctx, var->loc, VKD3D_SHADER_ERROR_HLSL_OVERLAPPING_RESERVATIONS,
> > -
> >                    "Multiple textures bound to t%u.", var->reg_reservation.index);
> > -
> >            hlsl_note(ctx, reserved_texture->loc, VKD3D_SHADER_LOG_ERROR,
> > -
> >                    "Texture '%s' is already bound to t%u.", reserved_texture->name,
> > -
> >                    var->reg_reservation.index);
> > -
> >        }
> > -
> > -
> >        var->reg.id = var->reg_reservation.index;
> > -
> >        var->reg.allocated = true;
> > -
> >        TRACE("Allocated reserved %s to t%u.\n", var->name, index);
> > -
> >    }
> > -
> >    else if (!var->reg_reservation.type)
> > -
> >    {
> > -
> >        while (get_reserved_texture(ctx, index))
> > -
> >            ++index;
> > -
> > -
> >        var->reg.id = index;
> > -
> >        var->reg.allocated = true;
> > -
> >        TRACE("Allocated %s to t%u.\n", var->name, index);
> > -
> >        ++index;
> > -
> >    }
> > -
> >    else
> > -
> >    {
> > -
> >        hlsl_error(ctx, var->loc, VKD3D_SHADER_ERROR_HLSL_INVALID_RESERVATION,
> > -
> >                "Textures must be bound to register type 't'.");
> > -
> >    }
> > - }
> >
> > +}
>
> The reservation lookup seems... suboptimal. It's not new (it's
> basically the same as what we do for buffers already) and probably
> there won't be that many extern variables in the usual cases, so maybe
> this is not going to be an issue in practice. It does kind of raise a
> red flag for me though...
> I guess let's keep this in mind if we ever happen to encounter
> performance issues.

I don't like how similar it looks to buffers (and samplers and UAVs), 
but it's not clear to me how to deduplicate it while not being horribly 
structured.

I certainly haven't seen shaders with a ton of externs yet.