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

[Martin Jambor]

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?)

Thanks,

Martin

>>Thanks,
>>
>>Martin
>>
>>
>>gcc/ChangeLog:
>>
>>2021-06-11  Martin Jambor  <mjambor@suse.cz>
>>
>>	PR tree-optimization/100453
>>	* tree-sra.c (create_access): Disqualify any const candidates
>>	which are written to.
>>	(sra_modify_expr): Do not store sub-replacements back to a const base.
>>	(handle_unscalarized_data_in_subtree): Likewise.
>>	(sra_modify_assign): Likewise.  Earlier, use TREE_READONLy test
>>	instead of constant_decl_p.
>>
>>gcc/testsuite/ChangeLog:
>>
>>2021-06-11  Martin Jambor  <mjambor@suse.cz>
>>
>>	PR tree-optimization/100453
>>	* gcc.dg/tree-ssa/pr100453.c: New test.