[PATCH vkd3d v2 1/3] vkd3d-shader/hlsl: Handle branches in copy propagation.
Zebediah Figura (she/her)
zfigura at codeweavers.com
Tue Jan 25 11:54:08 CST 2022
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?
More information about the wine-devel
mailing list