On Thu, Nov 18, 2021 at 10:09 AM Giovanni Mascellani gmascellani@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.