[PATCH vkd3d v6 5/6] vkd3d-shader/hlsl: Handle conditionals in copy propagation.

[Matteo Bruni]

On Thu, Nov 18, 2021 at 10:09 AM Giovanni Mascellani
<gmascellani at codeweavers.com> wrote:
>
> Hi,
>
> On 18/11/21 05:05, Zebediah Figura (she/her) wrote:
> > I don't think you even need to add any function parameters, do you?
> > Instead you do something like:
> >
> > struct copy_propagation_state
> > {
> >      struct rbtree variables;
> >      // if we want to avoid duplicating the contents:
> >      const struct copy_propagation_state *parent;
> > };
> >
> > static bool copy_propagation_handle_if(struct hlsl_ctx *ctx,
> >          struct hlsl_ir_if *iff, struct copy_propagation_state *parent)
> > {
> >      struct copy_propagation_state state;
> >
> >      rb_init(&state.variables);
> >
> >      // duplicate parent->variables into state.variables...
> >      // or, alternatively:
> >      state.parent = parent;
> > }
> >
> > Same number of parameters as we currently have.
> >
> > I'm not strongly attached to this solution, but I will say that coming
> > from a probably neutral perspective, it doesn't seem any more (or less?)
> > complex than what you have.
>
> Oh, I see what you mean.
>
> Personally, I still prefer my solution. It feels more idiomatic, you
> don't have to couple too much the stack in the compiler with the stack
> in the compiled program, so if you don't have a strong preference I
> would keep my way.

I don't know if that coupling you mention is necessarily a bad thing.
BTW, can anybody confirm that normally our stack should be able to
grow as needed?

> >>> One potential alternative that occurred to me: instead of duplicating
> >>> the whole state, we can search backwards in each parent scope in
> >>> copy_propagation_get_variable(). Notably, in the case of a loop, we'd
> >>> hit NULL before a "real" variable, and return NULL.
> >>>
> >>> Assuming that works it'd reduce the amount of allocation we have to do.
> >>
> >> I think it would work (I think I considered this approach at some
> >> point) and it would reduce the amount of allocation we do, but it
> >> would increase the amount of work that programmers have to do to write
> >> and maintain the code. Depending on the variable usage patterns, it
> >> could also increase the running time of the program. For these two
> >> reasons, mostly the first one, I don't think it is a good idea until
> >> stronger data emerges.
> >
> > Well, coming from a perspective that hasn't been working with either
> > variation, I'd say that they seem about equally natural to me. I don't
> > strongly prefer recursive search over duplication, though, so I don't
> > really intend to fight over this point.
>
> Here too I'd like to keep my way if you don't have a strong position on
> this.
>
> > Wrt CPU patterns, I'd be really surprised if searching parent scopes
> > ends up being more CPU-intensive than duplicating scope contents. You'd
> > have to be accessing each variable enough times to offset the cost of
> > memory allocation.
>
> On the other hand, each variable access with your solution requires many
> RB tree lookups, so the balance really depends on the usage patterns.

For better or worse, I thought the same things as Zebediah WRT this
patch. So I went ahead and reworked the patch along those lines. Let
me know if you don't like it / hate it.