ipa: Teach IPA-CP transformation about IPA-SRA modifications (PR 103227)

Message ID ri6h7c1iur8.fsf@suse.cz
State Committed
Commit 5bc4cb04127a4805b6228b0a6cbfebdbd61314d2
Headers
Series ipa: Teach IPA-CP transformation about IPA-SRA modifications (PR 103227) |

Commit Message

Martin Jambor Nov. 24, 2021, 12:49 p.m. UTC
  Hi,

PR 103227 exposed an issue with ordering of transformations of IPA
passes.  IPA-CP can create clones for constants passed by reference
and at the same time IPA-SRA can also decide that the parameter does
not need to be a pointer (or an aggregate) and plan to convert it
into (a) simple scalar(s).  Because no intermediate clone is created
just for the purpose of ordering the transformations and because
IPA-SRA transformation is implemented as part of clone
materialization, the IPA-CP transformation happens only afterwards,
reversing the order of the transformations compared to the ordering of
analyses.

IPA-CP transformation looks at planned substitutions for values passed
by reference or in aggregates but finds that all the relevant
parameters no longer exist.  Currently it subsequently simply gives
up, leading to clones created for no good purpose (and huge regression
of 548.exchange_r.  This patch teaches it recognize the situation,
look up the new scalarized parameter and perform value substitution on
it.  On my desktop this has recovered the lost exchange2 run-time (and
some more).

I have disabled IPA-SRA in a Fortran testcase so that the dumping from
the transformation phase can still be matched in order to verify that
IPA-CP understands the IL after verifying that it does the right thing
also with IPA-SRA.

Bootstrapped, LTO-bootstrapped and tested on x86_64.
Profiled-LTO-bootstrap on the same platform and normal bootstrap and
testing is underway on i686-linux and aarch64-linux.  OK all of that
passes as well?

Thanks,

Martin


gcc/ChangeLog:

2021-11-23  Martin Jambor  <mjambor@suse.cz>

	PR ipa/103227
	* ipa-prop.h (ipa_get_param): New overload.  Move bits of the existing
	one to the new one.
	* ipa-param-manipulation.h (ipa_param_adjustments): New member
	function get_updated_index_or_split.
	* ipa-param-manipulation.c
	(ipa_param_adjustments::get_updated_index_or_split): New function.
	* ipa-prop.c (adjust_agg_replacement_values): Reimplement, add
	capability to identify scalarized parameters and perform substitution
	on them.
	(ipcp_transform_function): Create descriptors earlier, handle new
	return values of adjust_agg_replacement_values.

gcc/testsuite/ChangeLog:

2021-11-23  Martin Jambor  <mjambor@suse.cz>

	PR ipa/103227
	* gcc.dg/ipa/pr103227-1.c: New test.
	* gcc.dg/ipa/pr103227-3.c: Likewise.
	* gcc.dg/ipa/pr103227-2.c: Likewise.
	* gfortran.dg/pr53787.f90: Disable IPA-SRA.
---
 gcc/ipa-param-manipulation.c          | 33 ++++++++++++
 gcc/ipa-param-manipulation.h          |  7 +++
 gcc/ipa-prop.c                        | 73 +++++++++++++++++++--------
 gcc/ipa-prop.h                        | 15 ++++--
 gcc/testsuite/gcc.dg/ipa/pr103227-1.c | 29 +++++++++++
 gcc/testsuite/gcc.dg/ipa/pr103227-2.c | 29 +++++++++++
 gcc/testsuite/gcc.dg/ipa/pr103227-3.c | 52 +++++++++++++++++++
 gcc/testsuite/gfortran.dg/pr53787.f90 |  2 +-
 8 files changed, 216 insertions(+), 24 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103227-1.c
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103227-2.c
 create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103227-3.c
  

Comments

Jan Hubicka Nov. 25, 2021, 11:14 a.m. UTC | #1
> 
> gcc/ChangeLog:
> 
> 2021-11-23  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR ipa/103227
> 	* ipa-prop.h (ipa_get_param): New overload.  Move bits of the existing
> 	one to the new one.
> 	* ipa-param-manipulation.h (ipa_param_adjustments): New member
> 	function get_updated_index_or_split.
> 	* ipa-param-manipulation.c
> 	(ipa_param_adjustments::get_updated_index_or_split): New function.
> 	* ipa-prop.c (adjust_agg_replacement_values): Reimplement, add
> 	capability to identify scalarized parameters and perform substitution
> 	on them.
> 	(ipcp_transform_function): Create descriptors earlier, handle new
> 	return values of adjust_agg_replacement_values.
> 
> gcc/testsuite/ChangeLog:
> 
> 2021-11-23  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR ipa/103227
> 	* gcc.dg/ipa/pr103227-1.c: New test.
> 	* gcc.dg/ipa/pr103227-3.c: Likewise.
> 	* gcc.dg/ipa/pr103227-2.c: Likewise.
> 	* gfortran.dg/pr53787.f90: Disable IPA-SRA.
> ---
>  gcc/ipa-param-manipulation.c          | 33 ++++++++++++
>  gcc/ipa-param-manipulation.h          |  7 +++
>  gcc/ipa-prop.c                        | 73 +++++++++++++++++++--------
>  gcc/ipa-prop.h                        | 15 ++++--
>  gcc/testsuite/gcc.dg/ipa/pr103227-1.c | 29 +++++++++++
>  gcc/testsuite/gcc.dg/ipa/pr103227-2.c | 29 +++++++++++
>  gcc/testsuite/gcc.dg/ipa/pr103227-3.c | 52 +++++++++++++++++++
>  gcc/testsuite/gfortran.dg/pr53787.f90 |  2 +-
>  8 files changed, 216 insertions(+), 24 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103227-1.c
>  create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103227-2.c
>  create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103227-3.c
> 
> diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
> index cec1dba701f..479c20b3871 100644
> --- a/gcc/ipa-param-manipulation.c
> +++ b/gcc/ipa-param-manipulation.c
> @@ -449,6 +449,39 @@ ipa_param_adjustments::get_updated_indices (vec<int> *new_indices)
>      }
>  }
>  
> +/* If a parameter with original INDEX has survived intact, return its new
> +   index.  Otherwise return -1.  In that case, if it has been split and there
> +   is a new parameter representing a portion at unit OFFSET for which a value
> +   of a TYPE can be substituted, store its new index into SPLIT_INDEX,
> +   otherwise store -1 there.  */
> +int
> +ipa_param_adjustments::get_updated_index_or_split (int index,
> +						   unsigned unit_offset,
> +						   tree type, int *split_index)
> +{
> +  unsigned adj_len = vec_safe_length (m_adj_params);
> +  for (unsigned i = 0; i < adj_len ; i++)

In ipa-modref I precompute this to map so we do not need to walk all
params, but the loop is probably not bad since functions do not have
tens of thousdands parameters :)

Can I use it in ipa-modref to discover what parameters was turned from
by-reference to scalar, too?
> +    {
> +      ipa_adjusted_param *apm = &(*m_adj_params)[i];
> +      if (apm->base_index != index)
> +	continue;
> +      if (apm->op == IPA_PARAM_OP_COPY)
> +	return i;
> +      if (apm->op == IPA_PARAM_OP_SPLIT
> +	  && apm->unit_offset == unit_offset)
> +	{
> +	  if (useless_type_conversion_p (apm->type, type))
> +	    *split_index = i;
> +	  else
> +	    *split_index = -1;
> +	  return -1;
> +	}
> +    }
> +
> +  *split_index = -1;
> +  return -1;
> +}
> +
>  /* Return the original index for the given new parameter index.  Return a
>     negative number if not available.  */
>  
> diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h
> index 5adf8a22356..d1dad9fac73 100644
> --- a/gcc/ipa-param-manipulation.h
> +++ b/gcc/ipa-param-manipulation.h
> @@ -236,6 +236,13 @@ public:
>    void get_surviving_params (vec<bool> *surviving_params);
>    /* Fill a vector with new indices of surviving original parameters.  */
>    void get_updated_indices (vec<int> *new_indices);
> +  /* If a parameter with original INDEX has survived intact, return its new
> +     index.  Otherwise return -1.  In that case, if it has been split and there
> +     is a new parameter representing a portion at UNIT_OFFSET for which a value
> +     of a TYPE can be substituted, store its new index into SPLIT_INDEX,
> +     otherwise store -1 there.  */
> +  int get_updated_index_or_split (int index, unsigned unit_offset, tree type,
> +				  int *split_index);
>    /* Return the original index for the given new parameter index.  Return a
>       negative number if not available.  */
>    int get_original_index (int newidx);
> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index e85df0971fc..a297f50e945 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -5578,32 +5578,55 @@ ipcp_read_transformation_summaries (void)
>  }
>  
>  /* Adjust the aggregate replacements in AGGVAL to reflect parameters skipped in
> -   NODE.  */
> +   NODE but also if any parameter was IPA-SRAed into a scalar go ahead with
> +   substitution of the default_definitions of that new param with the
> +   appropriate constant.
>  
> -static void
> -adjust_agg_replacement_values (struct cgraph_node *node,
> -			       struct ipa_agg_replacement_value *aggval)
> +   Return two bools.  the first it true if at least one item in AGGVAL still
> +   exists and function body walk should go ahead.  The second is true if any
> +   values were already substituted for scalarized parameters and update_cfg
> +   shuld be run after replace_uses_by.  */
> +
> +static std::pair<bool, bool>
What a fancy C++ :)
> +adjust_agg_replacement_values (cgraph_node *node,
> +			       ipa_agg_replacement_value *aggval,
> +			       const vec<ipa_param_descriptor, va_gc>
> +			         &descriptors)

Patch is OK.
Thanks,
Honza
  
Martin Jambor Nov. 25, 2021, 5:08 p.m. UTC | #2
Hi,

On Thu, Nov 25 2021, Jan Hubicka wrote:
>> 
>> gcc/ChangeLog:
>> 
>> 2021-11-23  Martin Jambor  <mjambor@suse.cz>
>> 
>> 	PR ipa/103227
>> 	* ipa-prop.h (ipa_get_param): New overload.  Move bits of the existing
>> 	one to the new one.
>> 	* ipa-param-manipulation.h (ipa_param_adjustments): New member
>> 	function get_updated_index_or_split.
>> 	* ipa-param-manipulation.c
>> 	(ipa_param_adjustments::get_updated_index_or_split): New function.
>> 	* ipa-prop.c (adjust_agg_replacement_values): Reimplement, add
>> 	capability to identify scalarized parameters and perform substitution
>> 	on them.
>> 	(ipcp_transform_function): Create descriptors earlier, handle new
>> 	return values of adjust_agg_replacement_values.
>> 
>> gcc/testsuite/ChangeLog:
>> 
>> 2021-11-23  Martin Jambor  <mjambor@suse.cz>
>> 
>> 	PR ipa/103227
>> 	* gcc.dg/ipa/pr103227-1.c: New test.
>> 	* gcc.dg/ipa/pr103227-3.c: Likewise.
>> 	* gcc.dg/ipa/pr103227-2.c: Likewise.
>> 	* gfortran.dg/pr53787.f90: Disable IPA-SRA.
>> ---
>>  gcc/ipa-param-manipulation.c          | 33 ++++++++++++
>>  gcc/ipa-param-manipulation.h          |  7 +++
>>  gcc/ipa-prop.c                        | 73 +++++++++++++++++++--------
>>  gcc/ipa-prop.h                        | 15 ++++--
>>  gcc/testsuite/gcc.dg/ipa/pr103227-1.c | 29 +++++++++++
>>  gcc/testsuite/gcc.dg/ipa/pr103227-2.c | 29 +++++++++++
>>  gcc/testsuite/gcc.dg/ipa/pr103227-3.c | 52 +++++++++++++++++++
>>  gcc/testsuite/gfortran.dg/pr53787.f90 |  2 +-
>>  8 files changed, 216 insertions(+), 24 deletions(-)
>>  create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103227-1.c
>>  create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103227-2.c
>>  create mode 100644 gcc/testsuite/gcc.dg/ipa/pr103227-3.c
>> 
>> diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
>> index cec1dba701f..479c20b3871 100644
>> --- a/gcc/ipa-param-manipulation.c
>> +++ b/gcc/ipa-param-manipulation.c
>> @@ -449,6 +449,39 @@ ipa_param_adjustments::get_updated_indices (vec<int> *new_indices)
>>      }
>>  }
>>  
>> +/* If a parameter with original INDEX has survived intact, return its new
>> +   index.  Otherwise return -1.  In that case, if it has been split and there
>> +   is a new parameter representing a portion at unit OFFSET for which a value
>> +   of a TYPE can be substituted, store its new index into SPLIT_INDEX,
>> +   otherwise store -1 there.  */
>> +int
>> +ipa_param_adjustments::get_updated_index_or_split (int index,
>> +						   unsigned unit_offset,
>> +						   tree type, int *split_index)
>> +{
>> +  unsigned adj_len = vec_safe_length (m_adj_params);
>> +  for (unsigned i = 0; i < adj_len ; i++)
>
> In ipa-modref I precompute this to map so we do not need to walk all
> params, but the loop is probably not bad since functions do not have
> tens of thousdands parameters :)

The most I have seen is about 70 and those were big outliers.

I was thinking of precomputing it somehow but for one parameter there
can be up to param ipa-sra-max-replacements replacements (default 8 -
and there is another, by default stricter, limit for pointers).  So it
would have to be a hash table or something like it.

>
> Can I use it in ipa-modref to discover what parameters was turned from
> by-reference to scalar, too?

IIUC, I don't think you directly can, also because for one parameter you
can have more scalar replacements and the interface needs an offset for
which to look.  OTOH, if you only care about simple scalars passed by
reference, then passing zero as offset - and probably adding a flag to
check there are no replacements at other offsets - would work.  (But
that information could also be easily pre-computed.)

>> +    {
>> +      ipa_adjusted_param *apm = &(*m_adj_params)[i];
>> +      if (apm->base_index != index)
>> +	continue;
>> +      if (apm->op == IPA_PARAM_OP_COPY)
>> +	return i;
>> +      if (apm->op == IPA_PARAM_OP_SPLIT
>> +	  && apm->unit_offset == unit_offset)
>> +	{
>> +	  if (useless_type_conversion_p (apm->type, type))
>> +	    *split_index = i;
>> +	  else
>> +	    *split_index = -1;
>> +	  return -1;
>> +	}
>> +    }
>> +
>> +  *split_index = -1;
>> +  return -1;
>> +}
>> +
>>  /* Return the original index for the given new parameter index.  Return a
>>     negative number if not available.  */
>>  
>> diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h
>> index 5adf8a22356..d1dad9fac73 100644
>> --- a/gcc/ipa-param-manipulation.h
>> +++ b/gcc/ipa-param-manipulation.h
>> @@ -236,6 +236,13 @@ public:
>>    void get_surviving_params (vec<bool> *surviving_params);
>>    /* Fill a vector with new indices of surviving original parameters.  */
>>    void get_updated_indices (vec<int> *new_indices);
>> +  /* If a parameter with original INDEX has survived intact, return its new
>> +     index.  Otherwise return -1.  In that case, if it has been split and there
>> +     is a new parameter representing a portion at UNIT_OFFSET for which a value
>> +     of a TYPE can be substituted, store its new index into SPLIT_INDEX,
>> +     otherwise store -1 there.  */
>> +  int get_updated_index_or_split (int index, unsigned unit_offset, tree type,
>> +				  int *split_index);
>>    /* Return the original index for the given new parameter index.  Return a
>>       negative number if not available.  */
>>    int get_original_index (int newidx);
>> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
>> index e85df0971fc..a297f50e945 100644
>> --- a/gcc/ipa-prop.c
>> +++ b/gcc/ipa-prop.c
>> @@ -5578,32 +5578,55 @@ ipcp_read_transformation_summaries (void)
>>  }
>>  
>>  /* Adjust the aggregate replacements in AGGVAL to reflect parameters skipped in
>> -   NODE.  */
>> +   NODE but also if any parameter was IPA-SRAed into a scalar go ahead with
>> +   substitution of the default_definitions of that new param with the
>> +   appropriate constant.
>>  
>> -static void
>> -adjust_agg_replacement_values (struct cgraph_node *node,
>> -			       struct ipa_agg_replacement_value *aggval)
>> +   Return two bools.  the first it true if at least one item in AGGVAL still
>> +   exists and function body walk should go ahead.  The second is true if any
>> +   values were already substituted for scalarized parameters and update_cfg
>> +   shuld be run after replace_uses_by.  */
>> +
>> +static std::pair<bool, bool>
> What a fancy C++ :)

Yeah :-) apparently I was spoiled by python this weekend.

>> +adjust_agg_replacement_values (cgraph_node *node,
>> +			       ipa_agg_replacement_value *aggval,
>> +			       const vec<ipa_param_descriptor, va_gc>
>> +			         &descriptors)
>
> Patch is OK.

Thanks a lot,

Martin
  
Jan Hubicka Nov. 25, 2021, 5:14 p.m. UTC | #3
> >
> > In ipa-modref I precompute this to map so we do not need to walk all
> > params, but the loop is probably not bad since functions do not have
> > tens of thousdands parameters :)
> 
> The most I have seen is about 70 and those were big outliers.
> 
> I was thinking of precomputing it somehow but for one parameter there
> can be up to param ipa-sra-max-replacements replacements (default 8 -
> and there is another, by default stricter, limit for pointers).  So it
> would have to be a hash table or something like it.

Yep, I think given that we have API, we can play with this later.
> 
> >
> > Can I use it in ipa-modref to discover what parameters was turned from
> > by-reference to scalar, too?
> 
> IIUC, I don't think you directly can, also because for one parameter you
> can have more scalar replacements and the interface needs an offset for
> which to look.  OTOH, if you only care about simple scalars passed by
> reference, then passing zero as offset - and probably adding a flag to
> check there are no replacements at other offsets - would work.  (But
> that information could also be easily pre-computed.)

If parameter is broken up into multipe pieces, I can just duplicate its
ECF flags (if I know that pointers from whole structure does not escape,
neither does pointers from its part). However presently modref compute
noting useful for aggregate parameters (have patch for that but it was
too late in this stage1 to push out everything, so it will come next
stage1).

If parameter is turned from by-reference and possibly offsetted, I can
use the original ECF flag after applying deref_flags translation.
Again it is not problem to multily it if the parameter is stplit into
multiple subparameters.

Honza
  

Patch

diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
index cec1dba701f..479c20b3871 100644
--- a/gcc/ipa-param-manipulation.c
+++ b/gcc/ipa-param-manipulation.c
@@ -449,6 +449,39 @@  ipa_param_adjustments::get_updated_indices (vec<int> *new_indices)
     }
 }
 
+/* If a parameter with original INDEX has survived intact, return its new
+   index.  Otherwise return -1.  In that case, if it has been split and there
+   is a new parameter representing a portion at unit OFFSET for which a value
+   of a TYPE can be substituted, store its new index into SPLIT_INDEX,
+   otherwise store -1 there.  */
+int
+ipa_param_adjustments::get_updated_index_or_split (int index,
+						   unsigned unit_offset,
+						   tree type, int *split_index)
+{
+  unsigned adj_len = vec_safe_length (m_adj_params);
+  for (unsigned i = 0; i < adj_len ; i++)
+    {
+      ipa_adjusted_param *apm = &(*m_adj_params)[i];
+      if (apm->base_index != index)
+	continue;
+      if (apm->op == IPA_PARAM_OP_COPY)
+	return i;
+      if (apm->op == IPA_PARAM_OP_SPLIT
+	  && apm->unit_offset == unit_offset)
+	{
+	  if (useless_type_conversion_p (apm->type, type))
+	    *split_index = i;
+	  else
+	    *split_index = -1;
+	  return -1;
+	}
+    }
+
+  *split_index = -1;
+  return -1;
+}
+
 /* Return the original index for the given new parameter index.  Return a
    negative number if not available.  */
 
diff --git a/gcc/ipa-param-manipulation.h b/gcc/ipa-param-manipulation.h
index 5adf8a22356..d1dad9fac73 100644
--- a/gcc/ipa-param-manipulation.h
+++ b/gcc/ipa-param-manipulation.h
@@ -236,6 +236,13 @@  public:
   void get_surviving_params (vec<bool> *surviving_params);
   /* Fill a vector with new indices of surviving original parameters.  */
   void get_updated_indices (vec<int> *new_indices);
+  /* If a parameter with original INDEX has survived intact, return its new
+     index.  Otherwise return -1.  In that case, if it has been split and there
+     is a new parameter representing a portion at UNIT_OFFSET for which a value
+     of a TYPE can be substituted, store its new index into SPLIT_INDEX,
+     otherwise store -1 there.  */
+  int get_updated_index_or_split (int index, unsigned unit_offset, tree type,
+				  int *split_index);
   /* Return the original index for the given new parameter index.  Return a
      negative number if not available.  */
   int get_original_index (int newidx);
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index e85df0971fc..a297f50e945 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -5578,32 +5578,55 @@  ipcp_read_transformation_summaries (void)
 }
 
 /* Adjust the aggregate replacements in AGGVAL to reflect parameters skipped in
-   NODE.  */
+   NODE but also if any parameter was IPA-SRAed into a scalar go ahead with
+   substitution of the default_definitions of that new param with the
+   appropriate constant.
 
-static void
-adjust_agg_replacement_values (struct cgraph_node *node,
-			       struct ipa_agg_replacement_value *aggval)
+   Return two bools.  the first it true if at least one item in AGGVAL still
+   exists and function body walk should go ahead.  The second is true if any
+   values were already substituted for scalarized parameters and update_cfg
+   shuld be run after replace_uses_by.  */
+
+static std::pair<bool, bool>
+adjust_agg_replacement_values (cgraph_node *node,
+			       ipa_agg_replacement_value *aggval,
+			       const vec<ipa_param_descriptor, va_gc>
+			         &descriptors)
 {
   struct ipa_agg_replacement_value *v;
   clone_info *cinfo = clone_info::get (node);
-
   if (!cinfo || !cinfo->param_adjustments)
-    return;
+    return std::pair<bool, bool> (true, false);
 
-  auto_vec<int, 16> new_indices;
-  cinfo->param_adjustments->get_updated_indices (&new_indices);
+  bool anything_left = false;
+  bool done_replacement = false;
   for (v = aggval; v; v = v->next)
     {
       gcc_checking_assert (v->index >= 0);
 
-      if ((unsigned) v->index < new_indices.length ())
-	v->index = new_indices[v->index];
-      else
-	/* This can happen if we know about a constant passed by reference by
-	   an argument which is never actually used for anything, let alone
-	   loading that constant.  */
-	v->index = -1;
+      unsigned unit_offset = v->offset / BITS_PER_UNIT;
+      tree cst_type = TREE_TYPE (v->value);
+      int split_idx;
+      int new_idx
+	= cinfo->param_adjustments->get_updated_index_or_split (v->index,
+								unit_offset,
+								cst_type,
+								&split_idx);
+      v->index = new_idx;
+      if (new_idx >= 0)
+	anything_left = true;
+      else if (split_idx >= 0)
+	{
+	  tree parm = ipa_get_param (descriptors, split_idx);
+	  tree ddef = ssa_default_def (cfun, parm);
+	  if (ddef)
+	    {
+	      replace_uses_by (ddef, v->value);
+	      done_replacement = true;
+	    }
+	}
     }
+   return std::pair<bool, bool> (anything_left, done_replacement);
 }
 
 /* Dominator walker driving the ipcp modification phase.  */
@@ -5995,7 +6018,19 @@  ipcp_transform_function (struct cgraph_node *node)
   param_count = count_formal_params (node->decl);
   if (param_count == 0)
     return 0;
-  adjust_agg_replacement_values (node, aggval);
+  vec_safe_grow_cleared (descriptors, param_count, true);
+  ipa_populate_param_decls (node, *descriptors);
+  std::pair<bool, bool> rr
+    = adjust_agg_replacement_values (node, aggval, *descriptors);
+  int retval = rr.second ? TODO_cleanup_cfg : 0;
+  if (!rr.first)
+    {
+      vec_free (descriptors);
+      if (dump_file)
+	fprintf (dump_file, "  All affected aggregate parameters were either "
+		 "removed or converted into scalars, phase done.\n");
+      return retval;
+    }
   if (dump_file)
     ipa_dump_agg_replacement_values (dump_file, aggval);
 
@@ -6006,8 +6041,6 @@  ipcp_transform_function (struct cgraph_node *node)
   fbi.param_count = param_count;
   fbi.aa_walk_budget = opt_for_fn (node->decl, param_ipa_max_aa_steps);
 
-  vec_safe_grow_cleared (descriptors, param_count, true);
-  ipa_populate_param_decls (node, *descriptors);
   calculate_dominance_info (CDI_DOMINATORS);
   ipcp_modif_dom_walker walker (&fbi, descriptors, aggval, &something_changed);
   walker.walk (ENTRY_BLOCK_PTR_FOR_FN (cfun));
@@ -6028,12 +6061,12 @@  ipcp_transform_function (struct cgraph_node *node)
   vec_free (descriptors);
 
   if (!something_changed)
-    return 0;
+    return retval;
 
   if (cfg_changed)
     delete_unreachable_blocks_update_callgraph (node, false);
 
-  return TODO_update_ssa_only_virtuals;
+  return retval | TODO_update_ssa_only_virtuals;
 }
 
 
diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
index 42842d9466a..caa947bc7c9 100644
--- a/gcc/ipa-prop.h
+++ b/gcc/ipa-prop.h
@@ -700,6 +700,17 @@  ipa_get_param_count (class ipa_node_params *info)
   return vec_safe_length (info->descriptors);
 }
 
+/* Return the parameter declaration in DESCRIPTORS at index I and assert it is
+   indeed a PARM_DECL.  */
+
+static inline tree
+ipa_get_param (const vec<ipa_param_descriptor, va_gc> &descriptors, int i)
+{
+  tree t = descriptors[i].decl_or_type;
+  gcc_checking_assert (TREE_CODE (t) == PARM_DECL);
+  return t;
+}
+
 /* Return the declaration of Ith formal parameter of the function corresponding
    to INFO.  Note there is no setter function as this array is built just once
    using ipa_initialize_node_params.  This function should not be called in
@@ -709,9 +720,7 @@  static inline tree
 ipa_get_param (class ipa_node_params *info, int i)
 {
   gcc_checking_assert (info->descriptors);
-  tree t = (*info->descriptors)[i].decl_or_type;
-  gcc_checking_assert (TREE_CODE (t) == PARM_DECL);
-  return t;
+  return ipa_get_param (*info->descriptors, i);
 }
 
 /* Return the type of Ith formal parameter of the function corresponding
diff --git a/gcc/testsuite/gcc.dg/ipa/pr103227-1.c b/gcc/testsuite/gcc.dg/ipa/pr103227-1.c
new file mode 100644
index 00000000000..0f56eb12179
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr103227-1.c
@@ -0,0 +1,29 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized-slim"  } */
+
+struct S
+{
+  int a, b, c;
+};
+
+int ellide (int c);
+
+static void __attribute__ ((noinline))
+foo (struct S *p)
+{
+  int c = p->c;
+  if (c != 21)
+    ellide (c);
+}
+
+void
+entry (void)
+{
+  struct S s;
+  s.a = 1;
+  s.b = 64;
+  s.c = 21;
+  foo (&s);
+}
+
+/* { dg-final { scan-tree-dump-not "ellide" "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/ipa/pr103227-2.c b/gcc/testsuite/gcc.dg/ipa/pr103227-2.c
new file mode 100644
index 00000000000..e4f3c715331
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr103227-2.c
@@ -0,0 +1,29 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized-slim"  } */
+
+struct S
+{
+  int a, b, c;
+};
+
+int ellide (int c);
+
+static void __attribute__ ((noinline))
+foo (struct S s)
+{
+  int c = s.c;
+  if (c != 21)
+    ellide (c);
+}
+
+void
+entry (void)
+{
+  struct S s;
+  s.a = 1;
+  s.b = 64;
+  s.c = 21;
+  foo (s);
+}
+
+/* { dg-final { scan-tree-dump-not "ellide" "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/ipa/pr103227-3.c b/gcc/testsuite/gcc.dg/ipa/pr103227-3.c
new file mode 100644
index 00000000000..a48026d1b95
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ipa/pr103227-3.c
@@ -0,0 +1,52 @@ 
+/* { dg-do run } */
+/* { dg-options "-O2 -fno-tree-fre -fno-tree-sra -fdump-tree-optimized-slim"  } */
+
+struct S
+{
+  int a, b, c;
+};
+
+volatile int z1;
+int z2 = 44;
+
+void  __attribute__((noipa))
+use_int (int c)
+{
+  z1 = c;
+}
+
+static void __attribute__ ((noinline))
+bar (struct S s)
+{
+  use_int (s.c);
+}
+
+
+static void __attribute__ ((noinline))
+foo (struct S s)
+{
+  int c = s.c;
+  if (c != 21)
+    use_int (c);
+
+  s.c = z2;
+  bar (s);
+  if (s.c != 44)
+    __builtin_abort ();
+}
+
+int
+main (void)
+{
+  struct S s;
+  s.a = 1;
+  s.b = 64;
+  s.c = 21;
+  foo (s);
+  return 0;
+}
+
+
+
+
+/* { dg-final { scan-tree-dump-not "ellide" "optimized" } } */
diff --git a/gcc/testsuite/gfortran.dg/pr53787.f90 b/gcc/testsuite/gfortran.dg/pr53787.f90
index f366a30bf59..412bbc7d7b0 100644
--- a/gcc/testsuite/gfortran.dg/pr53787.f90
+++ b/gcc/testsuite/gfortran.dg/pr53787.f90
@@ -1,5 +1,5 @@ 
 ! { dg-do compile }
-! { dg-options "-O3 -fdump-ipa-cp-details -fno-inline -fwhole-program" }
+! { dg-options "-O3 -fdump-ipa-cp-details -fno-ipa-sra -fno-inline -fwhole-program" }
 
   real x(10)
   n = 10