[PATCH vkd3d 6/7] vkd3d-shader/hlsl: Don't bother synthesizing a temp for input or output variables if they're not written to or read from, respectively.

[Zebediah Figura (she/her)]

On 10/26/21 07:48, Matteo Bruni wrote:
> On Fri, Oct 15, 2021 at 11:54 PM Zebediah Figura
> <zfigura at codeweavers.com> wrote:
>>
>> As a cheap substitute for actual variable copy-prop.
>>
>> Signed-off-by: Zebediah Figura <zfigura at codeweavers.com>
>> ---
>>   libs/vkd3d-shader/hlsl_codegen.c | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
>> index df10ca272..495ba7c47 100644
>> --- a/libs/vkd3d-shader/hlsl_codegen.c
>> +++ b/libs/vkd3d-shader/hlsl_codegen.c
>> @@ -34,6 +34,13 @@ static void prepend_uniform_copy(struct hlsl_ctx *ctx, struct list *instrs, stru
>>       /* Use the synthetic name for the temp, rather than the uniform, so that we
>>        * can write the uniform name into the shader reflection data. */
>>
>> +    if (!temp->first_write)
>> +    {
>> +        temp->is_uniform = 1;
>> +        list_add_tail(&ctx->extern_vars, &temp->extern_entry);
>> +        return;
>> +    }
>> +
>>       if (!(uniform = hlsl_new_var(ctx, temp->name, temp->data_type, temp->loc, NULL, 0, &temp->reg_reservation)))
>>           return;
>>       list_add_before(&temp->scope_entry, &uniform->scope_entry);
> 
> Technically this hunk is not covered by the patch subject.
> 
> I'm not sure I love these checks being in the functions rather than in
> the caller. It might just be about function naming... I guess what's
> bugging me is that, for "good" uniforms, prepend_uniform_copy() will
> NOT prepend any copy but still do something significant.

It's a fair cop.

I suppose it could be renamed to lower_uniform_writes() etc...

> 
>> @@ -67,6 +74,14 @@ static void prepend_input_copy(struct hlsl_ctx *ctx, struct list *instrs, struct
>>       struct hlsl_ir_load *load;
>>       struct hlsl_ir_var *input;
>>
>> +    /* We still need to split up non-vector types. */
>> +    if (!var->first_write && var->data_type->type <= HLSL_CLASS_VECTOR)
>> +    {
>> +        var->is_input_semantic = 1;
>> +        list_add_tail(&ctx->extern_vars, &var->extern_entry);
>> +        return;
>> +    }
>> +
>>       if (!(name = hlsl_get_string_buffer(ctx)))
>>           return;
>>       vkd3d_string_buffer_printf(name, "<input-%s%u>", semantic->name, semantic->index);
> 
> It's not obvious what splitting up input matrices (which is not there
> at all at the moment) has to do with adding a copy from an input
> variable.

Are input matrices even possible?

I'll have to clarify the comment in any case...

> 
>> @@ -138,6 +153,14 @@ static void append_output_copy(struct hlsl_ctx *ctx, struct list *instrs, struct
>>       struct hlsl_ir_var *output;
>>       struct hlsl_ir_load *load;
>>
>> +    /* We still need to split up non-vector types. */
>> +    if (!var->last_read && var->data_type->type <= HLSL_CLASS_VECTOR)
>> +    {
>> +        var->is_output_semantic = 1;
>> +        list_add_tail(&ctx->extern_vars, &var->extern_entry);
>> +        return;
>> +    }
>> +
>>       if (!(name = hlsl_get_string_buffer(ctx)))
>>           return;
>>       vkd3d_string_buffer_printf(name, "<output-%s%u>", semantic->name, semantic->index);
>> @@ -1306,6 +1329,12 @@ int hlsl_emit_dxbc(struct hlsl_ctx *ctx, struct hlsl_ir_function_decl *entry_fun
>>
>>       list_move_head(&body->instrs, &ctx->static_initializers);
>>
>> +    /* Do an initial liveness pass for externs so that we don't create
>> +     * unnecessary temps. Note that we might modify the instruction stream
>> +     * during the pass, but we're only checking whether a variable was read to
>> +     * or written from at all, so it's okay. */
>> +    compute_liveness(ctx, entry_func);
> 
> We shouldn't be modifying the instructions (aside from liveness data,
> of course) in compute_liveness(). I guess you mean that we can safely
> keep using liveness data that potentially becomes somewhat invalid
> during / after calling prepend_uniform_copy() /
> prepend_input_var_copy() / append_output_var_copy() but I'm having a
> hard time parsing the comment.
> 

Er, right, "the pass" means the pass after compute_liveness()...

Clearly this patch needs more clarity.