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?