From: "Zebediah Figura (she/her)" <zfigura@codeweavers.com>
Subject: Re: [PATCH vkd3d v2 1/3] vkd3d-shader/hlsl: Handle branches in copy propagation.
Message-Id: <29f7b1be-9375-ee12-a34c-ed2cb566a53f@codeweavers.com>
Date: Tue, 25 Jan 2022 11:54:08 -0600
In-Reply-To: <e372c083-04df-02a0-a5e6-eacebd015acc@codeweavers.com>
References: <20220120112349.365856-1-gmascellani@codeweavers.com> <7a3bf399-1dc1-cf72-0dd6-2f66d50dba80@codeweavers.com> <e372c083-04df-02a0-a5e6-eacebd015acc@codeweavers.com>

On 1/21/22 03:15, Giovanni Mascellani wrote:
> 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.

Ah, right, I can't read...

I would suggest folding the subsequent check for 
VALUE_STATE_STATICALLY_WRITTEN into this function, though.

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

I don't feel strongly about it, but we don't tend to bother zeroing 
invalidated pointers as a rule.

>>>   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()).

Ah, right. I probably knew that already but forgot.

In that case can you please add a comment so I don't forget again?