[PATCH] tree-inline: Fix TREE_READONLY of parameter replacements

Martin Jambor mjambor@suse.cz
Wed Jun 23 11:21:30 GMT 2021


Hi,

On Wed, Jun 23 2021, Richard Biener wrote:
> On Wed, 23 Jun 2021, Martin Jambor wrote:
>
>> Hi,
>> 
>> tree-inline leaves behind VAR_DECLs which are TREE_READONLY (because
>> they are copies of const parameters) but are written to because they
>> need to be initialized.  This patch resets the flag if any
>> initialization is performed, regardless of if the type needs
>> construction or not.
>
> I wonder if even that is premature optimization - it also removes
> a still useful comment.  So why not at the same place simply do
>
>   /* Even if P was TREE_READONLY, the new VAR should not be.
>      In the original code, we would have constructed a
>      temporary, and then the function body would have never
>      changed the value of P.  However, now, we will be
>      constructing VAR directly.  Therefore, it must not
>      be TREE_READONLY.  */ 
>   TREE_READONLY (var) = 0;
>
> ?  Since technically when in SSA form you wouldn't need to bother
> either, so your placement looks both premature and not good enough
> optimization.
>
> Did you check the above suggestion already and it failed for some
> reason?
>

No, I believe the above would work fine.  I just looked at the code and
the TREE_READONLY resetting is now just above two early exits which only
do an insert_init_debug_bind, so I thought it made sense to move the
reset of TREE_READONLY only to the same branch which actually generates
a write to var.

But only because I thought it would be more consistent, and "feel" more
logical, not as an optimization.  I do not think it makes any difference
in practice.  So if your feelings are different, I can certainly leave
it where it is.

As far as the comment is concerned, I like having comments about almost
everything, but this one seemed to me a bit too obvious, especially with
the TYPE_NEEDS_CONSTRUCTING condition removed.  But I can certainly keep
it too.

Martin



More information about the Gcc-patches mailing list