| Message ID | ri6sg1jp3kb.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | tree-sra: Do not refresh readonly decls (PR 100453) | expand |
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, > >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. >--- > gcc/testsuite/gcc.dg/tree-ssa/pr100453.c | 18 ++++++++++++++++++ > gcc/tree-sra.c | 21 +++++++++++++++++---- > 2 files changed, 35 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr100453.c > >diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr100453.c >b/gcc/testsuite/gcc.dg/tree-ssa/pr100453.c >new file mode 100644 >index 00000000000..0cf0ad23815 >--- /dev/null >+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr100453.c >@@ -0,0 +1,18 @@ >+/* { dg-do run } */ >+/* { dg-options "-O1" } */ >+ >+struct a { >+ int b : 4; >+} d; >+static int c, e; >+static const struct a f; >+static void g(const struct a h) { >+ for (; c < 1; c++) >+ d = h; >+ e = h.b; >+ c = h.b; >+} >+int main() { >+ g(f); >+ return 0; >+} >diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c >index 8dfc923ed7e..5e86d3fbb9d 100644 >--- a/gcc/tree-sra.c >+++ b/gcc/tree-sra.c >@@ -915,6 +915,12 @@ create_access (tree expr, gimple *stmt, bool >write) >if (!DECL_P (base) || !bitmap_bit_p (candidate_bitmap, DECL_UID >(base))) > return NULL; > >+ if (write && TREE_READONLY (base)) >+ { >+ disqualify_candidate (base, "Encountered a store to a read-only >decl."); >+ return NULL; >+ } >+ > HOST_WIDE_INT offset, size, max_size; > if (!poffset.is_constant (&offset) > || !psize.is_constant (&size) >@@ -3826,7 +3832,7 @@ sra_modify_expr (tree *expr, gimple_stmt_iterator >*gsi, bool write) > gsi_insert_after (gsi, ds, GSI_NEW_STMT); > } > >- if (access->first_child) >+ if (access->first_child && !TREE_READONLY (access->base)) > { > HOST_WIDE_INT start_offset, chunk_size; > if (bfr >@@ -3890,6 +3896,13 @@ static void >handle_unscalarized_data_in_subtree (struct >subreplacement_assignment_data *sad) > { > tree src; >+ /* If the RHS is a load from a constant, we do not need to (and must >not) >+ flush replacements to it and can use it directly as if we did. >*/ >+ if (TREE_READONLY (sad->top_racc->base)) >+ { >+ sad->refreshed = SRA_UDH_RIGHT; >+ return; >+ } > if (sad->top_racc->grp_unscalarized_data) > { > src = sad->assignment_rhs; >@@ -4243,8 +4256,8 @@ sra_modify_assign (gimple *stmt, >gimple_stmt_iterator *gsi) > || contains_vce_or_bfcref_p (lhs) > || stmt_ends_bb_p (stmt)) > { >- /* No need to copy into a constant-pool, it comes >pre-initialized. */ >- if (access_has_children_p (racc) && !constant_decl_p >(racc->base)) >+ /* No need to copy into a constant, it comes pre-initialized. >*/ >+ if (access_has_children_p (racc) && !TREE_READONLY (racc->base)) > generate_subtree_copies (racc->first_child, rhs, racc->offset, 0, 0, > gsi, false, false, loc); > if (access_has_children_p (lacc)) >@@ -4333,7 +4346,7 @@ sra_modify_assign (gimple *stmt, >gimple_stmt_iterator *gsi) > } > /* Restore the aggregate RHS from its components so the > prevailing aggregate copy does the right thing. */ >- if (access_has_children_p (racc)) >+ if (access_has_children_p (racc) && !TREE_READONLY (racc->base)) > generate_subtree_copies (racc->first_child, rhs, racc->offset, 0, >0, > gsi, false, false, loc); > /* Re-load the components of the aggregate copy destination.
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.
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. >
On Tue, Jun 15, 2021 at 06:11:27PM +0200, Richard Biener wrote: > >--- a/gcc/tree-sra.c > >+++ b/gcc/tree-sra.c > >@@ -915,6 +915,12 @@ create_access (tree expr, gimple *stmt, bool > >write) > >if (!DECL_P (base) || !bitmap_bit_p (candidate_bitmap, DECL_UID > >(base))) > > return NULL; > > > >+ if (write && TREE_READONLY (base)) > >+ { > >+ disqualify_candidate (base, "Encountered a store to a read-only > >decl."); Wouldn't this be a useful point to also emit some warning (with some TREE_NO_WARNING prevention) that some particular statement modifies a const decl? I guess it can be warned elsewhere though. As testcases one could use https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100994#c4 and #c5. Though would be nice if we diagnose that even without those -fno-* options. Jakub
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
On 6/16/21 5:48 AM, Jakub Jelinek wrote: > On Tue, Jun 15, 2021 at 06:11:27PM +0200, Richard Biener wrote: >>> --- a/gcc/tree-sra.c >>> +++ b/gcc/tree-sra.c >>> @@ -915,6 +915,12 @@ create_access (tree expr, gimple *stmt, bool >>> write) >>> if (!DECL_P (base) || !bitmap_bit_p (candidate_bitmap, DECL_UID >>> (base))) >>> return NULL; >>> >>> + if (write && TREE_READONLY (base)) >>> + { >>> + disqualify_candidate (base, "Encountered a store to a read-only >>> decl."); > > Wouldn't this be a useful point to also emit some warning (with > some TREE_NO_WARNING prevention) that some particular statement modifies > a const decl? > I guess it can be warned elsewhere though. > As testcases one could use https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100994#c4 > and #c5. Though would be nice if we diagnose that even without those -fno-* > options. I didn't finish my patch to diagnose these bugs in time for GCC 11 but I'm hoping to finish and submit it for GCC 12. (It's being tracked in PR 90404). My approach is the same as for similar warnings such as -Wfree-nonheap-object or -Wstringop-overflow. It depends on no particular optimization options (with all the expected consequences at -O0). Martin > > Jakub >
Hi, On Wed, Jun 16 2021, Jakub Jelinek wrote: > On Tue, Jun 15, 2021 at 06:11:27PM +0200, Richard Biener wrote: >> >--- a/gcc/tree-sra.c >> >+++ b/gcc/tree-sra.c >> >@@ -915,6 +915,12 @@ create_access (tree expr, gimple *stmt, bool >> >write) >> >if (!DECL_P (base) || !bitmap_bit_p (candidate_bitmap, DECL_UID >> >(base))) >> > return NULL; >> > >> >+ if (write && TREE_READONLY (base)) >> >+ { >> >+ disqualify_candidate (base, "Encountered a store to a read-only >> >decl."); > > Wouldn't this be a useful point to also emit some warning (with > some TREE_NO_WARNING prevention) that some particular statement modifies > a const decl? > I guess it can be warned elsewhere though. I would prefer it to be elsewhere. > As testcases one could use https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100994#c4 > and #c5. Though would be nice if we diagnose that even without those -fno-* > options. > My holy grail would be to actually add a condition to the gimple verifier that TREE_READONLY decl is never on a LHS of an assignment or a call (or generally not seen from the store callback of walk_stmt_load_store_addr_ops). Of course, in order to do that, either the gimplifier or the pass that propagated a TREE_READONLY decl into a store would have to drop the flag and at that point could emit a warning. But seeing things like the addition in a0d371a2514 I am afraid that getting there might be difficult. Martin
On Wed, 16 Jun 2021, Martin Jambor wrote: > Hi, > > On Wed, Jun 16 2021, Jakub Jelinek wrote: > > On Tue, Jun 15, 2021 at 06:11:27PM +0200, Richard Biener wrote: > >> >--- a/gcc/tree-sra.c > >> >+++ b/gcc/tree-sra.c > >> >@@ -915,6 +915,12 @@ create_access (tree expr, gimple *stmt, bool > >> >write) > >> >if (!DECL_P (base) || !bitmap_bit_p (candidate_bitmap, DECL_UID > >> >(base))) > >> > return NULL; > >> > > >> >+ if (write && TREE_READONLY (base)) > >> >+ { > >> >+ disqualify_candidate (base, "Encountered a store to a read-only > >> >decl."); > > > > Wouldn't this be a useful point to also emit some warning (with > > some TREE_NO_WARNING prevention) that some particular statement modifies > > a const decl? > > I guess it can be warned elsewhere though. > > I would prefer it to be elsewhere. > > > As testcases one could use https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100994#c4 > > and #c5. Though would be nice if we diagnose that even without those -fno-* > > options. > > > > My holy grail would be to actually add a condition to the gimple > verifier that TREE_READONLY decl is never on a LHS of an assignment or a > call (or generally not seen from the store callback of > walk_stmt_load_store_addr_ops). > > Of course, in order to do that, either the gimplifier or the pass that > propagated a TREE_READONLY decl into a store would have to drop the flag > and at that point could emit a warning. > > But seeing things like the addition in a0d371a2514 I am afraid that > getting there might be difficult. Fixing most easy places would still be a good idea. Eventually the OMP thing could be worked around by performing the verfication only after OMP lowering or so (at start). Richard.
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr100453.c b/gcc/testsuite/gcc.dg/tree-ssa/pr100453.c new file mode 100644 index 00000000000..0cf0ad23815 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr100453.c @@ -0,0 +1,18 @@ +/* { dg-do run } */ +/* { dg-options "-O1" } */ + +struct a { + int b : 4; +} d; +static int c, e; +static const struct a f; +static void g(const struct a h) { + for (; c < 1; c++) + d = h; + e = h.b; + c = h.b; +} +int main() { + g(f); + return 0; +} diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c index 8dfc923ed7e..5e86d3fbb9d 100644 --- a/gcc/tree-sra.c +++ b/gcc/tree-sra.c @@ -915,6 +915,12 @@ create_access (tree expr, gimple *stmt, bool write) if (!DECL_P (base) || !bitmap_bit_p (candidate_bitmap, DECL_UID (base))) return NULL; + if (write && TREE_READONLY (base)) + { + disqualify_candidate (base, "Encountered a store to a read-only decl."); + return NULL; + } + HOST_WIDE_INT offset, size, max_size; if (!poffset.is_constant (&offset) || !psize.is_constant (&size) @@ -3826,7 +3832,7 @@ sra_modify_expr (tree *expr, gimple_stmt_iterator *gsi, bool write) gsi_insert_after (gsi, ds, GSI_NEW_STMT); } - if (access->first_child) + if (access->first_child && !TREE_READONLY (access->base)) { HOST_WIDE_INT start_offset, chunk_size; if (bfr @@ -3890,6 +3896,13 @@ static void handle_unscalarized_data_in_subtree (struct subreplacement_assignment_data *sad) { tree src; + /* If the RHS is a load from a constant, we do not need to (and must not) + flush replacements to it and can use it directly as if we did. */ + if (TREE_READONLY (sad->top_racc->base)) + { + sad->refreshed = SRA_UDH_RIGHT; + return; + } if (sad->top_racc->grp_unscalarized_data) { src = sad->assignment_rhs; @@ -4243,8 +4256,8 @@ sra_modify_assign (gimple *stmt, gimple_stmt_iterator *gsi) || contains_vce_or_bfcref_p (lhs) || stmt_ends_bb_p (stmt)) { - /* No need to copy into a constant-pool, it comes pre-initialized. */ - if (access_has_children_p (racc) && !constant_decl_p (racc->base)) + /* No need to copy into a constant, it comes pre-initialized. */ + if (access_has_children_p (racc) && !TREE_READONLY (racc->base)) generate_subtree_copies (racc->first_child, rhs, racc->offset, 0, 0, gsi, false, false, loc); if (access_has_children_p (lacc)) @@ -4333,7 +4346,7 @@ sra_modify_assign (gimple *stmt, gimple_stmt_iterator *gsi) } /* Restore the aggregate RHS from its components so the prevailing aggregate copy does the right thing. */ - if (access_has_children_p (racc)) + if (access_has_children_p (racc) && !TREE_READONLY (racc->base)) generate_subtree_copies (racc->first_child, rhs, racc->offset, 0, 0, gsi, false, false, loc); /* Re-load the components of the aggregate copy destination.