| Message ID | ri6r0gbwf7l.fsf@virgil.suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | ipa: Avoid duplicate replacements in IPA-SRA transformation phase | expand |
On Fri, Mar 15, 2024 at 06:57:18PM +0100, Martin Jambor wrote: > --- a/gcc/ipa-param-manipulation.cc > +++ b/gcc/ipa-param-manipulation.cc > @@ -1525,6 +1525,22 @@ ipa_param_body_adjustments::common_initialization (tree old_fndecl, > replacement with a constant (for split aggregates passed > by value). */ > > + if (split[parm_num]) > + { > + /* We must be careful not to add a duplicate > + replacement. */ > + sort_replacements (); > + ipa_param_body_replacement *pbr = > + lookup_replacement_1 (m_oparms[parm_num], > + av.unit_offset); Formatting nit, the = should be on the next line before lookup_replacement_1. ipa_param_body_replacement *pbr = lookup_replacement_1 (m_oparms[parm_num], av.unit_offset); Jakub
Hello, and ping, please. (In my copy I have fixed the formatting issue spotted by Jakub.) Martin On Fri, Mar 15 2024, Martin Jambor wrote: > Hi, > > when the analysis part of IPA-SRA figures out that it would split out > a scalar part of an aggregate which is known by IPA-CP to contain a > known constant, it skips it knowing that the transformation part looks > at IPA-CP aggregate results too and does the right thing (which can > include doing the propagation in GIMPLE because that is the last > moment the parameter exists). > > However, when IPA-SRA wants to split out a smaller non-aggregate out > of an aggregate, which happens to be of the same size as a known > scalar constant at the same offset, the transformation bit fails to > recognize the situation, tries to do both splitting and constant > propagation and in PR 111571 testcase creates a nonsensical call > statement on which the call redirection then ICEs. > > Fixed by making sure we don't try to do two replacements of the same > part of the same parameter. > > The look-up among replacements requires these are sorted and this > patch just sorts them if they are not already sorted before each new > look-up. The worst number of sortings that can happen is number of > parameters which are both split and have aggregate constants times > param_ipa_max_agg_items (default 16). I don't think complicating the > source code to optimize for this unlikely case is worth it but if need > be, it can of course be done. > > Bootstrapped and tested on x86_64-linux. OK for master and eventually > also the gcc-13 branch? > > Thanks, > > Martin > > > > gcc/ChangeLog: > > 2024-03-15 Martin Jambor <mjambor@suse.cz> > > PR ipa/111571 > * ipa-param-manipulation.cc > (ipa_param_body_adjustments::common_initialization): Avoid creating > duplicate replacement entries. > > gcc/testsuite/ChangeLog: > > 2024-03-15 Martin Jambor <mjambor@suse.cz> > > PR ipa/111571 > * gcc.dg/ipa/pr111571.c: New test. > --- > gcc/ipa-param-manipulation.cc | 16 ++++++++++++++++ > gcc/testsuite/gcc.dg/ipa/pr111571.c | 29 +++++++++++++++++++++++++++++ > 2 files changed, 45 insertions(+) > create mode 100644 gcc/testsuite/gcc.dg/ipa/pr111571.c > > diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc > index 3e0df6a6f77..4c6337cc563 100644 > --- a/gcc/ipa-param-manipulation.cc > +++ b/gcc/ipa-param-manipulation.cc > @@ -1525,6 +1525,22 @@ ipa_param_body_adjustments::common_initialization (tree old_fndecl, > replacement with a constant (for split aggregates passed > by value). */ > > + if (split[parm_num]) > + { > + /* We must be careful not to add a duplicate > + replacement. */ > + sort_replacements (); > + ipa_param_body_replacement *pbr = > + lookup_replacement_1 (m_oparms[parm_num], > + av.unit_offset); > + if (pbr) > + { > + /* Otherwise IPA-SRA should have bailed out. */ > + gcc_assert (AGGREGATE_TYPE_P (TREE_TYPE (pbr->repl))); > + continue; > + } > + } > + > tree repl; > if (av.by_ref) > repl = av.value; > diff --git a/gcc/testsuite/gcc.dg/ipa/pr111571.c b/gcc/testsuite/gcc.dg/ipa/pr111571.c > new file mode 100644 > index 00000000000..2a4adc608db > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/ipa/pr111571.c > @@ -0,0 +1,29 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2" } */ > + > +struct a { > + int b; > +}; > +struct c { > + long d; > + struct a e; > + long f; > +}; > +int g, h, i; > +int j() {return 0;} > +static void k(struct a l, int p) { > + if (h) > + g = 0; > + for (; g; g = j()) > + if (l.b) > + break; > +} > +static void m(struct c l) { > + k(l.e, l.f); > + for (;; --i) > + ; > +} > +int main() { > + struct c n = {10, 9}; > + m(n); > +} > -- > 2.44.0
> > gcc/ChangeLog: > > > > 2024-03-15 Martin Jambor <mjambor@suse.cz> > > > > PR ipa/111571 > > * ipa-param-manipulation.cc > > (ipa_param_body_adjustments::common_initialization): Avoid creating > > duplicate replacement entries. > > > > gcc/testsuite/ChangeLog: > > > > 2024-03-15 Martin Jambor <mjambor@suse.cz> > > > > PR ipa/111571 > > * gcc.dg/ipa/pr111571.c: New test. OK, thanks! Honza
diff --git a/gcc/ipa-param-manipulation.cc b/gcc/ipa-param-manipulation.cc index 3e0df6a6f77..4c6337cc563 100644 --- a/gcc/ipa-param-manipulation.cc +++ b/gcc/ipa-param-manipulation.cc @@ -1525,6 +1525,22 @@ ipa_param_body_adjustments::common_initialization (tree old_fndecl, replacement with a constant (for split aggregates passed by value). */ + if (split[parm_num]) + { + /* We must be careful not to add a duplicate + replacement. */ + sort_replacements (); + ipa_param_body_replacement *pbr = + lookup_replacement_1 (m_oparms[parm_num], + av.unit_offset); + if (pbr) + { + /* Otherwise IPA-SRA should have bailed out. */ + gcc_assert (AGGREGATE_TYPE_P (TREE_TYPE (pbr->repl))); + continue; + } + } + tree repl; if (av.by_ref) repl = av.value; diff --git a/gcc/testsuite/gcc.dg/ipa/pr111571.c b/gcc/testsuite/gcc.dg/ipa/pr111571.c new file mode 100644 index 00000000000..2a4adc608db --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr111571.c @@ -0,0 +1,29 @@ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +struct a { + int b; +}; +struct c { + long d; + struct a e; + long f; +}; +int g, h, i; +int j() {return 0;} +static void k(struct a l, int p) { + if (h) + g = 0; + for (; g; g = j()) + if (l.b) + break; +} +static void m(struct c l) { + k(l.e, l.f); + for (;; --i) + ; +} +int main() { + struct c n = {10, 9}; + m(n); +}