[PATCH vkd3d 1/5] vkd3d-shader: Allocate temporary registers for variables.
Zebediah Figura (she/her)
zfigura at codeweavers.com
Fri Apr 2 17:07:18 CDT 2021
On 4/2/21 3:04 PM, Matteo Bruni wrote:
> On Wed, Mar 31, 2021 at 12:04 AM Zebediah Figura
> <zfigura at codeweavers.com> wrote:
>>
>> Signed-off-by: Zebediah Figura <zfigura at 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.
Yep...
(Can you tell this patch is very old?)
>
>> +{
>> + 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
Yeah, that was on my list to do 'eventually', but now is a good time :D
>
>> + {
>> + 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.
>
Assuming I understand correctly, I don't think it's necessary per se (we
may have to assign them to a temp reg for valid DXBC, but 3d22df25fe1
and friends already give us a temp variable for that purpose).
A more salient reason is that the variable itself can be modified in the
meantime, which in practice happens with postincrement.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20210402/b51a938c/attachment-0001.sig>
More information about the wine-devel
mailing list