Re: [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.

[Matteo Bruni]

On Fri, Oct 15, 2021 at 11:54 PM Zebediah Figura
 wrote:
> As a cheap substitute for actual variable copy-prop.
> Signed-off-by: Zebediah Figura
>
> 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.
> @@ -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.
> @@ -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.