On Wed, Mar 31, 2021 at 12:04 AM Zebediah Figura <zfigura(a)codeweavers.com> wrote:
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com> --- 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.