From: Giovanni Mascellani <gmascellani@codeweavers.com>
Subject: Re: [PATCH vkd3d v2 1/3] vkd3d-shader/hlsl: Handle branches in copy propagation.
Message-Id: <e372c083-04df-02a0-a5e6-eacebd015acc@codeweavers.com>
Date: Fri, 21 Jan 2022 10:15:09 +0100
In-Reply-To: <7a3bf399-1dc1-cf72-0dd6-2f66d50dba80@codeweavers.com>
References: <20220120112349.365856-1-gmascellani@codeweavers.com> <7a3bf399-1dc1-cf72-0dd6-2f66d50dba80@codeweavers.com>

Hi,

Il 21/01/22 03:19, Zebediah Figura (she/her) ha scritto:
>> -    if (entry)
>> -        return RB_ENTRY_VALUE(entry, struct copy_propagation_var_def, 
>> entry);
>> -    else
>> -        return NULL;
>> +            if (var_def->values[component].state != 
>> VALUE_STATE_NOT_WRITTEN)
> 
> Why not just check for VALUE_STATE_STATICALLY_WRITTEN?

It's not the same thing. If the value has been dynamically written I 
have to stop searching, it's the answer the caller needs to get. 
Otherwise I risk returning an outer statically written value enabling an 
invalid optimization.

>> +    for (i = 0; i < 4; ++i)
>> +    {
>> +        if (writemask & (1u << i))
>> +        {
>> +            memset(&var_def->values[offset + i], 0, 
>> sizeof(var_def->values[offset + i]));
> 
> This memset() doesn't seem necessary anymore.

I agree it is not, given that when the value is not statically written, 
node and component should not even be checked. I thought it would be 
cleaner to reset them to zero anyway (e.g., it might help debugging in 
the future), but if it bothers you I can remove it.

>>   static void copy_propagation_invalidate_whole_variable(struct 
>> copy_propagation_var_def *var_def)
>>   {
>> +    unsigned i;
>> +
>>       TRACE("Invalidate variable %s.\n", var_def->var->name);
>> +
>>       memset(var_def->values, 0, sizeof(*var_def->values) * 
>> var_def->var->data_type->reg_size);
> 
> Note that you don't need this memset() anymore either.

Same as above.

> As an alternative, you could iterate over the whole state, marking the 
> parent as "dynamically written" wherever the child state is written.

That's what I did in my first implementation, and from some point of 
view it would be nicer, but it doesn't play well with loops, because 
there you have to know what you are going to invalidate before iterating 
into the loop itself. Having two different ways to invalidate the outer 
state looks like a useless amount of code to maintain, so I think it is 
better to just do this way (copy_propagation_invalidate_from_block()).

Giovanni.