Re: [PATCH vkd3d 1/5] vkd3d-shader: Allocate temporary registers for variables.

[Matteo Bruni]

On Wed, Mar 31, 2021 at 12:04 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 | 184 +++++++++++++++++++++++++++++++
>  3 files changed, 195 insertions(+), 2 deletions(-)

This code feels strangely familiar ;)

I have some nitpicks, as usual.
> diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> index dbd591af..420e720f 100644
> --- a/libs/vkd3d-shader/hlsl_codegen.c
> +++ b/libs/vkd3d-shader/hlsl_codegen.c
> @@ -612,6 +612,188 @@ static void compute_liveness(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl
>      compute_liveness_recurse(entry_func->body, 0, 0);
>  }
>
> +struct liveness
> +{
> - size_t size;
> - struct
> - {
> -
>    /* 0 if not live yet. */
> -
>    unsigned int last_read;
> - } *regs;
>
> +};
> -
>
> +static unsigned char get_available_writemask(struct liveness *liveness,
> -
>    unsigned int first_write, unsigned int index, unsigned int components)
>
> +{
> - unsigned char i, writemask = 0, count = 0;
> -
> - for (i = 0; i < 4; ++i)
> - {
> -
>    if (liveness->regs[index + i].last_read <= first_write)
> -
>    {
> -
>        writemask |= 1 << i;
> -
>        if (++count == components)
> -
>            return writemask;
> -
>    }
> - }
> -
> - return 0;
>
> +}
> -
>
> +static bool resize_liveness(struct liveness *liveness, size_t new_count)
> +{
> - size_t old_capacity = liveness->size;
> -
> - if (!vkd3d_array_reserve((void **)&liveness->regs, &liveness->size, new_count, sizeof(*liveness->regs)))
> -
>    return false;
> -
> - if (liveness->size > old_capacity)
> -
>    memset(liveness->regs + old_capacity, 0, (liveness->size - old_capacity) * sizeof(*liveness->regs));
> - return true;
>
> +}
> -
>
> +static struct hlsl_reg allocate_register(struct liveness *liveness,
> -
>    unsigned int first_write, unsigned int last_read, unsigned char components)

This is the only instance of "components" (which actually should be
"component_count") that isn't unsigned int. I assume there was some
back and forth while writing the patch.
> +{
> - struct hlsl_reg ret = {.allocated = true};
> - unsigned char writemask, i;
> - unsigned int regnum;

No reason to use an unsigned char for writemask or i. "regnum" should
probably be "index" (or "base_index", or "base", or something).
> -
> - for (regnum = 0; regnum < liveness->size; regnum += 4)
> - {
> -
>    if ((writemask = get_available_writemask(liveness, first_write, regnum, components)))
> -
>        break;
> - }
> - if (regnum == liveness->size)
> - {
> -
>    if (!resize_liveness(liveness, regnum + 4))
> -
>        return ret;
> -
>    writemask = (1 << components) - 1;

1u (also elsewhere).
> - }
> - for (i = 0; i < 4; ++i)
> - {
> -
>    if (writemask & (1 << i))
> -
>        liveness->regs[regnum + i].last_read = last_read;
> - }
> - ret.id = regnum / 4;
> - ret.writemask = writemask;
> - return ret;
>
> +}
> -
>
> +static bool is_range_available(struct liveness *liveness, unsigned int first_write,
> -
>    unsigned int index, unsigned int elements)
>
> +{
> - unsigned int i;
> -
> - for (i = 0; i < elements; i += 4)
> - {
> -
>    if (!get_available_writemask(liveness, first_write, index + i, 4))
> -
>        return false;
> - }
> - return true;
>
> +}
> -
>
> +/* "elements" is the total number of consecutive whole registers needed. */

Let's call it register_count or element_count. Or just count.
> +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", debug_register('r', var->reg, var->data_type),
> -
>            var->name, var->first_write, var->last_read);
> - }

I guess both work, but it sounds better to me as "Allocated <variable
name> to <register>".
> +}
> -
>
> +static void allocate_temp_registers_recurse(struct list *instrs, struct liveness *liveness)
> +{
> - struct hlsl_ir_node *instr;
> -
> - LIST_FOR_EACH_ENTRY(instr, instrs, struct hlsl_ir_node, entry)
> - {
> -
>    switch (instr->type)
> -
>    {
> -
>        case HLSL_IR_ASSIGNMENT:

Entirely an aside: ASSIGNMENT looks pretty ugly now that we have LOAD
for the symmetric operation and that the IR has been reworked into a
somewhat proper load / store architecture.
Up to you when (or if) to do a compiler-wide rename, just know that
every time I read HLSL_IR_ASSIGNMENT I feel a tiiiiiiny bit of pain :P
> -
>        {
> -
>            struct hlsl_ir_assignment *assignment = hlsl_ir_assignment(instr);
> -
>            allocate_variable_temp_register(assignment->lhs.var, liveness);
> -
>            break;
> -
>        }
> -
> -
>        case HLSL_IR_IF:
> -
>        {
> -
>            struct hlsl_ir_if *iff = hlsl_ir_if(instr);
> -
>            allocate_temp_registers_recurse(&iff->then_instrs, liveness);
> -
>            allocate_temp_registers_recurse(&iff->else_instrs, liveness);
> -
>            break;
> -
>        }
> -
> -
>        case HLSL_IR_LOAD:
> -
>        {
> -
>            struct hlsl_ir_load *load = hlsl_ir_load(instr);
> -
>            allocate_variable_temp_register(load->src.var, liveness);
> -
>            break;
> -
>        }

This one is required because of input arguments and uniforms, I
gather. Maybe a comment would be useful? I am not sure, honestly.