Message ID | ri6y2b3dlgr.fsf@suse.cz |
---|---|
State | New |
Headers | show |
Series | ipa-sra: Fix thinko when overriding safe_to_import_accesses (PR 101066) | expand |
Ping. On Mon, Jun 21 2021, Martin Jambor wrote: > Hi, > > The "new" IPA-SRA has a more difficult job than the previous > not-truly-IPA version when identifying situations in which a parameter > passed by reference can be passed into a third function and only thee > converted to one passed by value (and possibly "split" at the same > time). > > In order to allow this, two conditions must be fulfilled. First the > call to the third function must happen before any modifications of > memory, because it could change the value passed by reference. > Second, in order to make sure we do not introduce new (invalid) > dereferences, the call must postdominate the entry BB. > > The second condition is actually not necessary if the caller function > is also certain to dereference the pointer but the first one must > still hold. Unfortunately, the code making this overriding decision > also happen to trigger when the first condition is not fulfilled. > This is fixed in the following patch. > > Bootstrapped, LTO-bootstrapped and tested on x86_64-linux, OK for trunk > and the gcc-11 branch? On gcc-10, I might just remove the override > altogether, the case might not be important enough to change LTO format. > > Thanks, > > Martin > > > > gcc/ChangeLog: > > 2021-06-16 Martin Jambor <mjambor@suse.cz> > > PR ipa/101066 > * ipa-sra.c (class isra_call_summary): New member > m_before_any_store, initialize it in the constructor. > (isra_call_summary::dump): Dump the new field. > (ipa_sra_call_summaries::duplicate): Copy it. > (process_scan_results): Set it. > (isra_write_edge_summary): Stream it. > (isra_read_edge_summary): Likewise. > (param_splitting_across_edge): Only override > safe_to_import_accesses if m_before_any_store is set. > > gcc/testsuite/ChangeLog: > > 2021-06-16 Martin Jambor <mjambor@suse.cz> > > PR ipa/101066 > * gcc.dg/ipa/pr101066.c: New test. > --- > gcc/ipa-sra.c | 15 +++++++++++++-- > gcc/testsuite/gcc.dg/ipa/pr101066.c | 20 ++++++++++++++++++++ > 2 files changed, 33 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/ipa/pr101066.c > > diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c > index 3272daf56e4..965e246d788 100644 > --- a/gcc/ipa-sra.c > +++ b/gcc/ipa-sra.c > @@ -343,7 +343,7 @@ class isra_call_summary > public: > isra_call_summary () > : m_arg_flow (), m_return_ignored (false), m_return_returned (false), > - m_bit_aligned_arg (false) > + m_bit_aligned_arg (false), m_before_any_store (false) > {} > > void init_inputs (unsigned arg_count); > @@ -362,6 +362,10 @@ public: > > /* Set when any of the call arguments are not byte-aligned. */ > unsigned m_bit_aligned_arg : 1; > + > + /* Set to true if the call happend before any (other) store to memory in the > + caller. */ > + unsigned m_before_any_store : 1; > }; > > /* Class to manage function summaries. */ > @@ -491,6 +495,8 @@ isra_call_summary::dump (FILE *f) > fprintf (f, " return value ignored\n"); > if (m_return_returned) > fprintf (f, " return value used only to compute caller return value\n"); > + if (m_before_any_store) > + fprintf (f, " happens before any store to memory\n"); > for (unsigned i = 0; i < m_arg_flow.length (); i++) > { > fprintf (f, " Parameter %u:\n", i); > @@ -535,6 +541,7 @@ ipa_sra_call_summaries::duplicate (cgraph_edge *, cgraph_edge *, > new_sum->m_return_ignored = old_sum->m_return_ignored; > new_sum->m_return_returned = old_sum->m_return_returned; > new_sum->m_bit_aligned_arg = old_sum->m_bit_aligned_arg; > + new_sum->m_before_any_store = old_sum->m_before_any_store; > } > > > @@ -2374,6 +2381,7 @@ process_scan_results (cgraph_node *node, struct function *fun, > unsigned count = gimple_call_num_args (call_stmt); > isra_call_summary *csum = call_sums->get_create (cs); > csum->init_inputs (count); > + csum->m_before_any_store = uses_memory_as_obtained; > for (unsigned argidx = 0; argidx < count; argidx++) > { > if (!csum->m_arg_flow[argidx].pointer_pass_through) > @@ -2546,6 +2554,7 @@ isra_write_edge_summary (output_block *ob, cgraph_edge *e) > bp_pack_value (&bp, csum->m_return_ignored, 1); > bp_pack_value (&bp, csum->m_return_returned, 1); > bp_pack_value (&bp, csum->m_bit_aligned_arg, 1); > + bp_pack_value (&bp, csum->m_before_any_store, 1); > streamer_write_bitpack (&bp); > } > > @@ -2664,6 +2673,7 @@ isra_read_edge_summary (struct lto_input_block *ib, cgraph_edge *cs) > csum->m_return_ignored = bp_unpack_value (&bp, 1); > csum->m_return_returned = bp_unpack_value (&bp, 1); > csum->m_bit_aligned_arg = bp_unpack_value (&bp, 1); > + csum->m_before_any_store = bp_unpack_value (&bp, 1); > } > > /* Read intraprocedural analysis information about NODE and all of its outgoing > @@ -3420,7 +3430,8 @@ param_splitting_across_edge (cgraph_edge *cs) > } > else if (!ipf->safe_to_import_accesses) > { > - if (!all_callee_accesses_present_p (param_desc, arg_desc)) > + if (!csum->m_before_any_store > + || !all_callee_accesses_present_p (param_desc, arg_desc)) > { > if (dump_file && (dump_flags & TDF_DETAILS)) > fprintf (dump_file, " %u->%u: cannot import accesses.\n", > diff --git a/gcc/testsuite/gcc.dg/ipa/pr101066.c b/gcc/testsuite/gcc.dg/ipa/pr101066.c > new file mode 100644 > index 00000000000..1ceb6e43136 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/ipa/pr101066.c > @@ -0,0 +1,20 @@ > +/* { dg-do run } */ > +/* { dg-options "-Os -fno-ipa-cp -fno-inline" } */ > + > +int a = 1, c, d, e; > +int *b = &a; > +static int g(int *h) { > + c = *h; > + return d; > +} > +static void f(int *h) { > + e = *h; > + *b = 0; > + g(h); > +} > +int main() { > + f(b); > + if (c) > + __builtin_abort(); > + return 0; > +} > -- > 2.31.1
Hi, > 2021-06-16 Martin Jambor <mjambor@suse.cz> > > PR ipa/101066 > * ipa-sra.c (class isra_call_summary): New member > m_before_any_store, initialize it in the constructor. > (isra_call_summary::dump): Dump the new field. > (ipa_sra_call_summaries::duplicate): Copy it. > (process_scan_results): Set it. > (isra_write_edge_summary): Stream it. > (isra_read_edge_summary): Likewise. > (param_splitting_across_edge): Only override > safe_to_import_accesses if m_before_any_store is set. > > gcc/testsuite/ChangeLog: > > 2021-06-16 Martin Jambor <mjambor@suse.cz> > > PR ipa/101066 > * gcc.dg/ipa/pr101066.c: New test. OK, thanks! The analysis disabling transformation on any memory store is overly conservative. We have pointer (which is a parameter and comes from outer world) and no type infomration, however alias oracle will still be able to disambiguate when memory access is to non-escaping local memory or mallocated memory block etc. Honza
diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c index 3272daf56e4..965e246d788 100644 --- a/gcc/ipa-sra.c +++ b/gcc/ipa-sra.c @@ -343,7 +343,7 @@ class isra_call_summary public: isra_call_summary () : m_arg_flow (), m_return_ignored (false), m_return_returned (false), - m_bit_aligned_arg (false) + m_bit_aligned_arg (false), m_before_any_store (false) {} void init_inputs (unsigned arg_count); @@ -362,6 +362,10 @@ public: /* Set when any of the call arguments are not byte-aligned. */ unsigned m_bit_aligned_arg : 1; + + /* Set to true if the call happend before any (other) store to memory in the + caller. */ + unsigned m_before_any_store : 1; }; /* Class to manage function summaries. */ @@ -491,6 +495,8 @@ isra_call_summary::dump (FILE *f) fprintf (f, " return value ignored\n"); if (m_return_returned) fprintf (f, " return value used only to compute caller return value\n"); + if (m_before_any_store) + fprintf (f, " happens before any store to memory\n"); for (unsigned i = 0; i < m_arg_flow.length (); i++) { fprintf (f, " Parameter %u:\n", i); @@ -535,6 +541,7 @@ ipa_sra_call_summaries::duplicate (cgraph_edge *, cgraph_edge *, new_sum->m_return_ignored = old_sum->m_return_ignored; new_sum->m_return_returned = old_sum->m_return_returned; new_sum->m_bit_aligned_arg = old_sum->m_bit_aligned_arg; + new_sum->m_before_any_store = old_sum->m_before_any_store; } @@ -2374,6 +2381,7 @@ process_scan_results (cgraph_node *node, struct function *fun, unsigned count = gimple_call_num_args (call_stmt); isra_call_summary *csum = call_sums->get_create (cs); csum->init_inputs (count); + csum->m_before_any_store = uses_memory_as_obtained; for (unsigned argidx = 0; argidx < count; argidx++) { if (!csum->m_arg_flow[argidx].pointer_pass_through) @@ -2546,6 +2554,7 @@ isra_write_edge_summary (output_block *ob, cgraph_edge *e) bp_pack_value (&bp, csum->m_return_ignored, 1); bp_pack_value (&bp, csum->m_return_returned, 1); bp_pack_value (&bp, csum->m_bit_aligned_arg, 1); + bp_pack_value (&bp, csum->m_before_any_store, 1); streamer_write_bitpack (&bp); } @@ -2664,6 +2673,7 @@ isra_read_edge_summary (struct lto_input_block *ib, cgraph_edge *cs) csum->m_return_ignored = bp_unpack_value (&bp, 1); csum->m_return_returned = bp_unpack_value (&bp, 1); csum->m_bit_aligned_arg = bp_unpack_value (&bp, 1); + csum->m_before_any_store = bp_unpack_value (&bp, 1); } /* Read intraprocedural analysis information about NODE and all of its outgoing @@ -3420,7 +3430,8 @@ param_splitting_across_edge (cgraph_edge *cs) } else if (!ipf->safe_to_import_accesses) { - if (!all_callee_accesses_present_p (param_desc, arg_desc)) + if (!csum->m_before_any_store + || !all_callee_accesses_present_p (param_desc, arg_desc)) { if (dump_file && (dump_flags & TDF_DETAILS)) fprintf (dump_file, " %u->%u: cannot import accesses.\n", diff --git a/gcc/testsuite/gcc.dg/ipa/pr101066.c b/gcc/testsuite/gcc.dg/ipa/pr101066.c new file mode 100644 index 00000000000..1ceb6e43136 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr101066.c @@ -0,0 +1,20 @@ +/* { dg-do run } */ +/* { dg-options "-Os -fno-ipa-cp -fno-inline" } */ + +int a = 1, c, d, e; +int *b = &a; +static int g(int *h) { + c = *h; + return d; +} +static void f(int *h) { + e = *h; + *b = 0; + g(h); +} +int main() { + f(b); + if (c) + __builtin_abort(); + return 0; +}