Re: [PATCH vkd3d v3 2/6] vkd3d-shader: Allocate temporary registers for variables.

[Matteo Bruni]

On Fri, Apr 9, 2021 at 6:38 AM Zebediah Figura  wrote:
> Signed-off-by: Zebediah Figura
>
> libs/vkd3d-shader/hlsl.c         |   4 +-
>  libs/vkd3d-shader/hlsl.h         |   9 ++
>  libs/vkd3d-shader/hlsl_codegen.c | 187 +++++++++++++++++++++++++++++++
>  3 files changed, 198 insertions(+), 2 deletions(-)

I keep finding nitpicks one at a time...
> +static struct hlsl_reg allocate_range(struct liveness *liveness,
> -
>    unsigned int first_write, unsigned int last_read, unsigned int reg_count)
>
> +{
> - const unsigned int component_count = reg_count * 4;
> - unsigned int i, component_idx;
> - struct hlsl_reg ret = {0};
> -
> - for (component_idx = 0; component_idx < liveness->size; component_idx += 4)
> - {
> -
>    if (is_range_available(liveness, first_write, component_idx,
> -
>            min(component_count, liveness->size - component_idx)))
> -
>        break;
> - }
> - if (!resize_liveness(liveness, component_idx + component_count))
> -
>    return ret;
> -
> - for (i = 0; i < component_count; ++i)
> -
>    liveness->regs[component_idx + i].last_read = last_read;
> - ret.id = component_idx / 4;
> - ret.allocated = true;
> - return ret;
>
> +}

This is slightly awkward in that it sets writemask to 0. That is
arguably correct (more than 1 full register is allocated, in which
case there is no partial register allocation) but we have to be
careful with the users of struct hlsl_reg. At the moment there is not
much of that, just the TRACE() in allocate_variable_temp_register()
that prints something like "r0.". Speaking of which...
> -
>
> +static const char *debug_register(char class, struct hlsl_reg reg, const struct hlsl_type *type)
> +{
> - if (type->reg_size > 4)
> -
>    return vkd3d_dbg_sprintf("%c%u-%c%u", class, reg.id, class,
> -
>            reg.id + type->reg_size - 1);
> - return vkd3d_dbg_sprintf("%c%u%s", class, reg.id, debug_hlsl_writemask(reg.writemask));
>
> +}
> -
>
> +static void allocate_variable_temp_register(struct hlsl_ir_var *var, struct liveness *liveness)
> +{
> - if (var->is_input_varying || var->is_output_varying || var->is_uniform)
> -
>    return;
> -
> - if (!var->reg.allocated && var->last_read)
> - {
> -
>    if (var->data_type->reg_size > 1)
> -
>        var->reg = allocate_range(liveness, var->first_write,
> -
>                var->last_read, var->data_type->reg_size);
> -
>    else
> -
>        var->reg = allocate_register(liveness, var->first_write,
> -
>                var->last_read, var->data_type->dimx);
> -
>    TRACE("Allocated %s to %s (liveness %u-%u).\n", var->name,
> -
>            debug_register('r', var->reg, var->data_type), var->first_write, var->last_read);

This doesn't tell the whole story for register ranges, maybe we could
print more info in that case.