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 <mjam...@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 <mjam...@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 <mjam...@suse.cz> > >> > >> PR tree-optimization/100453 > >> * gcc.dg/tree-ssa/pr100453.c: New test. > -- Richard Biener <rguent...@suse.de> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
