ipa-sra: Avoid clashes with ipa-cp when pulling accesses across calls (PR 118243)

  Hello, 

among other things, IPA-SRA checks whether splitting out a bit of an
aggregate or something passed by reference would lead into a clash
with an already known IPA-CP constant a way which would cause problems
later on.  Unfortunately the test is done only in
adjust_parameter_descriptions and is missing when accesses are
propagated from callees to callers, which leads to miscompilation
reported as PR 118243 (where the callee is a function created by
ipa-split).

The matter is then further complicated by the fact that we consider
complex numbers as scalars even though they can be modified piecemeal
(IPA-CP can detect and propagate the pieces separately too) which then
confuses the parameter manipulation machinery furter.

This patch simply adds the missing check to avoid the IPA-SRA
transform in these cases too, which should be suitable for backporting
to all affected release branches.  It is a bit of a shame as in the PR
testcase we do propagate both components of the complex number in
question and the transformation phase could recover.  I have some
prototype patches in this direction but that is something for (a)
stage 1.

Bootstrapped and tested on x86_64-linux.  OK for master and (assuming it
applies cleanly and passes the checks there too) to all active release
branches?

Thanks,

Martin


gcc/ChangeLog:

2025-02-10  Martin Jambor  <mjambor@suse.cz>

	PR ipa/118243
	* ipa-sra.cc (pull_accesses_from_callee): New parameters
	caller_ipcp_ts and param_idx.  Check that scalar pulled accesses would
	not clash with a known IPA-CP aggregate constant.
	(param_splitting_across_edge): Pass IPA-CP transformation summary and
	caller parameter index to pull_accesses_from_callee.

gcc/testsuite/ChangeLog:

2025-02-10  Martin Jambor  <mjambor@suse.cz>

	PR ipa/118243
	* g++.dg/ipa/pr118243.C: New test.
---
 gcc/ipa-sra.cc                      | 38 +++++++++++++++++++--------
 gcc/testsuite/g++.dg/ipa/pr118243.C | 40 +++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 10 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr118243.C
  

Hello,

and ping please.

Thanks,

Martin


On Mon, Feb 10 2025, Martin Jambor wrote:
> Hello, 
>
> among other things, IPA-SRA checks whether splitting out a bit of an
> aggregate or something passed by reference would lead into a clash
> with an already known IPA-CP constant a way which would cause problems
> later on.  Unfortunately the test is done only in
> adjust_parameter_descriptions and is missing when accesses are
> propagated from callees to callers, which leads to miscompilation
> reported as PR 118243 (where the callee is a function created by
> ipa-split).
>
> The matter is then further complicated by the fact that we consider
> complex numbers as scalars even though they can be modified piecemeal
> (IPA-CP can detect and propagate the pieces separately too) which then
> confuses the parameter manipulation machinery furter.
>
> This patch simply adds the missing check to avoid the IPA-SRA
> transform in these cases too, which should be suitable for backporting
> to all affected release branches.  It is a bit of a shame as in the PR
> testcase we do propagate both components of the complex number in
> question and the transformation phase could recover.  I have some
> prototype patches in this direction but that is something for (a)
> stage 1.
>
> Bootstrapped and tested on x86_64-linux.  OK for master and (assuming it
> applies cleanly and passes the checks there too) to all active release
> branches?
>
> Thanks,
>
> Martin
>
>
> gcc/ChangeLog:
>
> 2025-02-10  Martin Jambor  <mjambor@suse.cz>
>
> 	PR ipa/118243
> 	* ipa-sra.cc (pull_accesses_from_callee): New parameters
> 	caller_ipcp_ts and param_idx.  Check that scalar pulled accesses would
> 	not clash with a known IPA-CP aggregate constant.
> 	(param_splitting_across_edge): Pass IPA-CP transformation summary and
> 	caller parameter index to pull_accesses_from_callee.
>
> gcc/testsuite/ChangeLog:
>
> 2025-02-10  Martin Jambor  <mjambor@suse.cz>
>
> 	PR ipa/118243
> 	* g++.dg/ipa/pr118243.C: New test.
> ---
>  gcc/ipa-sra.cc                      | 38 +++++++++++++++++++--------
>  gcc/testsuite/g++.dg/ipa/pr118243.C | 40 +++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+), 10 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr118243.C
>
> diff --git a/gcc/ipa-sra.cc b/gcc/ipa-sra.cc
> index ad80d22f8ce..5d1703ed394 100644
> --- a/gcc/ipa-sra.cc
> +++ b/gcc/ipa-sra.cc
> @@ -3640,15 +3640,19 @@ enum acc_prop_kind {ACC_PROP_DONT, ACC_PROP_COPY, ACC_PROP_CERTAIN};
>  
>  /* Attempt to propagate all definite accesses from ARG_DESC to PARAM_DESC,
>     (which belongs to CALLER) if they would not violate some constraint there.
> -   If successful, return NULL, otherwise return the string reason for failure
> -   (which can be written to the dump file).  DELTA_OFFSET is the known offset
> -   of the actual argument withing the formal parameter (so of ARG_DESCS within
> -   PARAM_DESCS), ARG_SIZE is the size of the actual argument or zero, if not
> -   known. In case of success, set *CHANGE_P to true if propagation actually
> -   changed anything.  */
> +   CALLER_IPCP_TS describes the caller, PARAM_IDX is the index of the parameter
> +   described by PARAM_DESC.  If successful, return NULL, otherwise return the
> +   string reason for failure (which can be written to the dump file).
> +   DELTA_OFFSET is the known offset of the actual argument withing the formal
> +   parameter (so of ARG_DESCS within PARAM_DESCS), ARG_SIZE is the size of the
> +   actual argument or zero, if not known. In case of success, set *CHANGE_P to
> +   true if propagation actually changed anything.  */
>  
>  static const char *
> -pull_accesses_from_callee (cgraph_node *caller, isra_param_desc *param_desc,
> +pull_accesses_from_callee (cgraph_node *caller,
> +			   ipcp_transformation *caller_ipcp_ts,
> +			   int param_idx,
> +			   isra_param_desc *param_desc,
>  			   isra_param_desc *arg_desc,
>  			   unsigned delta_offset, unsigned arg_size,
>  			   bool *change_p)
> @@ -3673,6 +3677,17 @@ pull_accesses_from_callee (cgraph_node *caller, isra_param_desc *param_desc,
>  	continue;
>  
>        unsigned offset = argacc->unit_offset + delta_offset;
> +
> +      if (caller_ipcp_ts && !AGGREGATE_TYPE_P (argacc->type))
> +	{
> +	  ipa_argagg_value_list avl (caller_ipcp_ts);
> +	  tree value = avl.get_value (param_idx, offset);
> +	  if (value && ((tree_to_uhwi (TYPE_SIZE (TREE_TYPE (value)))
> +			 / BITS_PER_UNIT)
> +			!= argacc->unit_size))
> +	    return " propagated access would conflict with an IPA-CP constant";
> +	}
> +
>        /* Given that accesses are initially stored according to increasing
>  	 offset and decreasing size in case of equal offsets, the following
>  	 searches could be written more efficiently if we kept the ordering
> @@ -3781,6 +3796,8 @@ param_splitting_across_edge (cgraph_edge *cs)
>    cgraph_node *callee = cs->callee->function_symbol (&availability);
>    isra_func_summary *from_ifs = func_sums->get (cs->caller);
>    gcc_checking_assert (from_ifs && from_ifs->m_parameters);
> +  ipcp_transformation *caller_ipcp_ts
> +    = ipcp_get_transformation_summary (cs->caller);
>  
>    isra_call_summary *csum = call_sums->get (cs);
>    gcc_checking_assert (csum);
> @@ -3851,8 +3868,8 @@ param_splitting_across_edge (cgraph_edge *cs)
>  	  else
>  	    {
>  	      const char *pull_failure
> -		= pull_accesses_from_callee (cs->caller, param_desc, arg_desc,
> -					     0, 0, &res);
> +		= pull_accesses_from_callee (cs->caller, caller_ipcp_ts, idx,
> +					     param_desc, arg_desc, 0, 0, &res);
>  	      if (pull_failure)
>  		{
>  		  if (dump_file && (dump_flags & TDF_DETAILS))
> @@ -3913,7 +3930,8 @@ param_splitting_across_edge (cgraph_edge *cs)
>  	  else
>  	    {
>  	      const char *pull_failure
> -		= pull_accesses_from_callee (cs->caller, param_desc, arg_desc,
> +		= pull_accesses_from_callee (cs->caller, caller_ipcp_ts, idx,
> +					     param_desc, arg_desc,
>  					     ipf->unit_offset,
>  					     ipf->unit_size, &res);
>  	      if (pull_failure)
> diff --git a/gcc/testsuite/g++.dg/ipa/pr118243.C b/gcc/testsuite/g++.dg/ipa/pr118243.C
> new file mode 100644
> index 00000000000..5efaa276da1
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ipa/pr118243.C
> @@ -0,0 +1,40 @@
> +/* { dg-do run } */
> +/* { dg-options "-O3 -std=gnu++11" } */
> +
> +using complex_t = int __complex__;
> +
> +struct A {
> +    complex_t value;
> +    A(double r) : value{0, r} {}
> +};
> +
> +[[gnu::noipa]]
> +void f(int a)
> +{
> +  if (a != 1)
> +    __builtin_abort();
> +}
> +[[gnu::noipa]] void g(const char *, int x){}
> +
> +
> +void test(const complex_t &c, const int &x) {
> +    if (x < 0)
> +        g("%d\n", x);
> +    else
> +    {
> +        f( __imag__ c);
> +    }
> +}
> +
> +void f1() {
> +    {
> +        A a{1};
> +        test(a.value, 123);
> +    }
> +}
> +
> +int main()
> +{
> +        f1();
> +	return 0;
> +}
> -- 
> 2.47.1
  

> gcc/ChangeLog:
>
> 2025-02-10  Martin Jambor  <mjambor@suse.cz>
>
> 	PR ipa/118243
> 	* ipa-sra.cc (pull_accesses_from_callee): New parameters
> 	caller_ipcp_ts and param_idx.  Check that scalar pulled accesses would
> 	not clash with a known IPA-CP aggregate constant.
> 	(param_splitting_across_edge): Pass IPA-CP transformation summary and
> 	caller parameter index to pull_accesses_from_callee.
>
> gcc/testsuite/ChangeLog:
>
> 2025-02-10  Martin Jambor  <mjambor@suse.cz>
>
> 	PR ipa/118243
> 	* g++.dg/ipa/pr118243.C: New test.

OK,
thanks!
Honza