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