[PATCH vkd3d v3 16/17] vkd3d-shader/hlsl: Replace register offsets with index paths in input/output copies.

[Matteo Bruni]

On Thu, Aug 4, 2022 at 9:10 PM Francisco Casas <fcasas at codeweavers.com> wrote:
>
> Hello,
>
> On 28-07-22 16:59, Matteo Bruni wrote:
> > On Wed, Jul 20, 2022 at 3:24 PM Francisco Casas <fcasas at codeweavers.com> wrote:
> >>
> >> Signed-off-by: Francisco Casas <fcasas at codeweavers.com>
> >> ---
> >>
> >> v3:
> >> * No changes.
> >>
> >> The recursive structure of prepend_input_var_copy() and
> >> append_output_var_copy() could be preserved creating additional
> >> loads to complete the paths. Otherwise we would be requiring
> >> passing whole paths as arguments.
> >>
> >> These additional loads should be handled by DCE.
> >>
> >> Still, matrix vectors are copied iteratively instead of recursively now,
> >> to avoid the boilerplate of creating new loads in this last step.
> >>
> >> Signed-off-by: Francisco Casas <fcasas at codeweavers.com>
> >> ---
> >>   libs/vkd3d-shader/hlsl_codegen.c | 188 +++++++++++++++++++------------
> >>   1 file changed, 116 insertions(+), 72 deletions(-)
> >>
> >> diff --git a/libs/vkd3d-shader/hlsl_codegen.c b/libs/vkd3d-shader/hlsl_codegen.c
> >> index 6437006b..7a245007 100644
> >> --- a/libs/vkd3d-shader/hlsl_codegen.c
> >> +++ b/libs/vkd3d-shader/hlsl_codegen.c
> >> @@ -239,59 +239,75 @@ static struct hlsl_ir_var *add_semantic_var(struct hlsl_ctx *ctx, struct hlsl_ir
> >>       return ext_var;
> >>   }
> >>
> >> -static void prepend_input_copy(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_var *var,
> >> -        struct hlsl_type *type, unsigned int field_offset, unsigned int modifiers, const struct hlsl_semantic *semantic)
> >> +static void prepend_input_copy(struct hlsl_ctx *ctx, struct list *instrs, struct hlsl_ir_load *lhs,
> >> +        struct hlsl_type *type, unsigned int modifiers, const struct hlsl_semantic *semantic)
> >
> > I haven't put a lot of thought into this, but it seems to me we could
> > pass a deref instead of a redundant load and be mostly set. Mostly
> > because we then need to add the instructions to the instruction list
> > in some other way (e.g. by passing an explicit list).
>
> I tried it passing a hlsl_blocks to be initialized (like in the
> component load/store initializers) but in the end it didn't seem like a
> good idea to me:
>
> The problem is that we would need to create derefs in the wild (outside
> hlsl_ir_nodes), which would mean exposing several static functions in
> hlsl.c, like init_deref_from_component_index(). We would also need to
> use "hlsl_cleanup_deref()" which for now is only non-static because it
> is needed in the the pass that translates index paths back to register
> offsets.
>
> So I don't think exposing the deref functions is better than adding
> these redundant loads, which are later deleted by the DCE pass, if I am
> not mistaken.
>
> Alternatively, we could create the nodes just as a container for the
> derefs and immediately free them, but that also seems a little ugly.
>
> So my vote is to keep this signature. Maybe remove the "type" parameter
> since it can be obtained from the deref inside the load node.

Fair enough. That was hard to see without actually trying and it
sounds like redundant loads are the lesser evil in this case. Maybe a
comment somewhere explaining that these loads are redundant and
supposed to be eliminated by DCE would be nice.