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

[Giovanni Mascellani]

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 at 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.