[PATCH] tree-sra: Do not refresh readonly decls (PR 100453)

Jeff Law jeffreyalaw@gmail.com
Wed Jun 16 14:47:57 GMT 2021



On 6/16/2021 4:00 AM, Richard Biener wrote:
> On Wed, 16 Jun 2021, Martin Jambor wrote:
>
>> Hi Richi,
>>
>> On Tue, Jun 15 2021, Richard Biener wrote:
>>> On June 15, 2021 5:09:40 PM GMT+02:00, Martin Jambor <mjambor@suse.cz> wrote:
>>>> Hi,
>>>>
>>>> When SRA transforms an assignment where the RHS is an aggregate decl
>>>> that it creates replacements for, the (least efficient) fallback method
>>>> of dealing with them is to store all the replacements back into the
>>>> original decl and then let the original assignment takes its course.
>>>>
>>>> That of course should not need to be done for TREE_READONLY bases which
>>>> cannot change contents.  The SRA code handled this situation only for
>>>> DECL_IN_CONSTANT_POOL const decls, this patch modifies the check so
>>>> that
>>>> it tests for TREE_READONLY and I also looked at all other callers of
>>>> generate_subtree_copies and added checks to another one dealing with
>>>> the
>>>> same exact situation and one which deals with it in a non-assignment
>>>> context.
>>>>
>>>> This behavior also means that SRA has to disqualify any candidate decl
>>>> that is read-only and written to.  I plan to continue to hunt down at
>>>> least some of such occurrences.
>>>>
>>>> Bootstrapped and tested on x86_64-linux, i686-linux and aarch64-linux
>>>> (this time With Ada enabled on all three platforms).  OK for trunk?
>>> Ok.
>>>
>>> Thanks,
>>> Richard.
>>>
>> Thanks for a quick approval.  However, when looking for sources of
>> additional non-read-only TREE_READONLY decls, I found the following code
>> and comment in setup_one_parameter() in tree-inline.c, and the last
>> comment sentence made me wonder if my patch is perhaps too strict:
>>
>>    /* 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.  The constructor body may
>>       change its value multiple times as it is being
>>       constructed.  Therefore, it must not be TREE_READONLY;
>>       the back-end assumes that TREE_READONLY variable is
>>       assigned to only once.  */
>>    if (TYPE_NEEDS_CONSTRUCTING (TREE_TYPE (p)))
>>      TREE_READONLY (var) = 0;
>>
>> Is the last sentence in the comment true?  Do we want it to be true?  It
>> contradicts the description of TREE_READONLY in tree.h.  (Would the
>> described property ever be useful in the middle-end or back-end?)
> I think the last sentence refers to RTX_UNCHANGING_P which we thankfully
> removed.  Now, that means we need to clear TREE_READONLY unconditionally
> here I think (unless we can prove it's uninitialized in the caller,
> but I guess we don't need to prematurely optimize that case).
Yea, I suspect that TREE_READONLY would morph into RTX_UNCHANGING_P 
which we did assume was written only once and it was nothing but trouble.
jeff



More information about the Gcc-patches mailing list