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

Richard Biener rguenther@suse.de
Wed Jun 16 10:00:37 GMT 2021


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

Richard.

> 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.
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)


More information about the Gcc-patches mailing list