Hi,
On 17/11/21 05:41, Zebediah Figura (she/her) wrote:
On 11/16/21 13:00, Giovanni Mascellani wrote:
Signed-off-by: Giovanni Mascellani gmascellani@codeweavers.com
libs/vkd3d-shader/hlsl_codegen.c | 157 +++++++++++++++++++++++++++++-- 1 file changed, 149 insertions(+), 8 deletions(-)
I'd been putting off these two patches because I thought they would be harder and I wanted to get the basic parts in first, but I had a look at them anyway and they are actually pretty simple. So, nice job :-)
Good, thanks for the review! :-)
@@ -272,7 +283,9 @@ struct copy_propagation_variable struct copy_propagation_state { - struct rb_tree variables; + struct rb_tree *variables; + unsigned int depth; + unsigned int capacity; };
As an alternative, you could create a new copy_propagation_state struct on stack in copy_propagation_process_if().
I guess I could, but I don't like this very much: it would mean that I have to pass more pointers between calls, which makes the code more convoluted and refactoring more difficult. The advantage of putting all the state in a single structure is that you just pass a pointer to that structure, and in if the future you want to extend that, you minimize the impact on internal interfaces.
Also, it's easier (or so I think) for someone who reads the code for the first time to understand what's going on, because all the state is listed in a single place, and you don't need to understand all the function call graph before you can start making sense of what is being stored.
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.
The other comments will be addressed in v7.
Giovanni.