[PR,67794] Also remap SSA_NAMEs defined in ASMs in IPA-SRA

Hi,

the following fixes PR 67794 by properly remapping SSA_NAMEs which are
based on PARM_DECLs which are about to be removed as unnecessary.  And
by "properly" I mean also when they are defined by a GIMPL_ASM
statement.  In fact, it switches to using an iterator over definitions
to make sure it always handles everything...  well,except for PHIs
which are still handled specially because, from a quick glance over
their source, it seemed to me that the iterator does not support them.

Bootstrapped and tested on x86_64-linux.  OK for trunk?
The issue is most probably latent on a number of old branches, do we
want to backport the patch to any of them?

Thanks,

Martin


2015-10-08  Martin Jambor  <mjambor@suse.cz>

	tree-optimization/67794
	* tree-sra.c (replace_removed_params_ssa_names): Do not distinguish
	between types of state,ents but accept original definitions as a
	parameter.
	(ipa_sra_modify_function_body): Use FOR_EACH_SSA_DEF_OPERAND to
	iterate over definitions.

testsuite/
        * gcc.dg/ipa/ipa-sra-10.c: Nw test.
        * gcc.dg/torture/pr67794.c: Likewise.

On Thu, 8 Oct 2015, Martin Jambor wrote:

> Hi,
> 
> the following fixes PR 67794 by properly remapping SSA_NAMEs which are
> based on PARM_DECLs which are about to be removed as unnecessary.  And
> by "properly" I mean also when they are defined by a GIMPL_ASM
> statement.  In fact, it switches to using an iterator over definitions
> to make sure it always handles everything...  well,except for PHIs
> which are still handled specially because, from a quick glance over
> their source, it seemed to me that the iterator does not support them.
> 
> Bootstrapped and tested on x86_64-linux.  OK for trunk?
> The issue is most probably latent on a number of old branches, do we
> want to backport the patch to any of them?
> 
> Thanks,
> 
> Martin
> 
> 
> 2015-10-08  Martin Jambor  <mjambor@suse.cz>
> 
> 	tree-optimization/67794
> 	* tree-sra.c (replace_removed_params_ssa_names): Do not distinguish
> 	between types of state,ents but accept original definitions as a
> 	parameter.
> 	(ipa_sra_modify_function_body): Use FOR_EACH_SSA_DEF_OPERAND to
> 	iterate over definitions.
> 
> testsuite/
>         * gcc.dg/ipa/ipa-sra-10.c: Nw test.
>         * gcc.dg/torture/pr67794.c: Likewise.
> 
> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-10.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-10.c
> new file mode 100644
> index 0000000..24b64d1
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-10.c
> @@ -0,0 +1,34 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fipa-sra -fdump-tree-eipa_sra-details"  } */
> +
> +extern void consume (int);
> +extern int glob, glob1, glob2;
> +extern int get (void);
> +
> +
> +static void __attribute__ ((noinline))
> +foo (int a)
> +{
> +  a = glob;
> +  consume (a);
> +  a = get ();
> +  consume (a);
> +  __asm__ volatile("" : : ""(a));
> +  consume (a);
> +
> +  if (glob1)
> +    a = glob1;
> +  else
> +    a = glob2;
> +  consume (a);
> +}
> +
> +int
> +bar (int a)
> +{
> +  foo (a);
> +  glob = a;
> +  return 0;
> +}
> +
> +/* { dg-final { scan-tree-dump-times "replacing an SSA name of a removed param" 4 "eipa_sra" } } */
> diff --git a/gcc/testsuite/gcc.dg/torture/pr67794.c b/gcc/testsuite/gcc.dg/torture/pr67794.c
> new file mode 100644
> index 0000000..5489e56
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr67794.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +
> +int *b;
> +static void fn1(int *best, int *dmin) {
> +  int a[64];
> +  dmin = a;
> +  __asm__ volatile("" : "+&r"(dmin) : ""(best));
> +}
> +
> +__attribute__((always_inline)) static inline void fn2(int *best) { fn1(best, b); }
> +
> +void fn3(void) {
> +  int c[1];
> +  fn2(c);
> +}
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index 4327990..f2a4e72 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -4612,61 +4612,45 @@ get_adjustment_for_base (ipa_parm_adjustment_vec adjustments, tree base)
>    return NULL;
>  }
>  
> -/* If the statement STMT defines an SSA_NAME of a parameter which is to be
> -   removed because its value is not used, replace the SSA_NAME with a one
> -   relating to a created VAR_DECL together all of its uses and return true.
> -   ADJUSTMENTS is a pointer to an adjustments vector.  */
> +/* If OLD_NAME, which is being defined by statement STMT, is an SSA_NAME of a
> +   parameter which is to be removed because its value is not used, create a new
> +   SSA_NAME relating to a replacement VAR_DECL, replace all uses of the
> +   original with it and return it.  If there is no need to re-map, return true.
> +   ADJUSTMENTS is a pointer to a vector of IPA-SRA adjustments.  */

The docs still say we return a bool

Otherwise the patch looks ok to me.  I think we want to backport it
to branche(s) after a while.

Thanks,
Richard.

> -static bool
> -replace_removed_params_ssa_names (gimple *stmt,
> +static tree
> +replace_removed_params_ssa_names (tree old_name, gimple *stmt,
>  				  ipa_parm_adjustment_vec adjustments)
>  {
>    struct ipa_parm_adjustment *adj;
> -  tree lhs, decl, repl, name;
> -
> -  if (gimple_code (stmt) == GIMPLE_PHI)
> -    lhs = gimple_phi_result (stmt);
> -  else if (is_gimple_assign (stmt))
> -    lhs = gimple_assign_lhs (stmt);
> -  else if (is_gimple_call (stmt))
> -    lhs = gimple_call_lhs (stmt);
> -  else
> -    gcc_unreachable ();
> +  tree decl, repl, new_name;
>  
> -  if (TREE_CODE (lhs) != SSA_NAME)
> -    return false;
> +  if (TREE_CODE (old_name) != SSA_NAME)
> +    return NULL;
>  
> -  decl = SSA_NAME_VAR (lhs);
> +  decl = SSA_NAME_VAR (old_name);
>    if (decl == NULL_TREE
>        || TREE_CODE (decl) != PARM_DECL)
> -    return false;
> +    return NULL;
>  
>    adj = get_adjustment_for_base (adjustments, decl);
>    if (!adj)
> -    return false;
> +    return NULL;
>  
>    repl = get_replaced_param_substitute (adj);
> -  name = make_ssa_name (repl, stmt);
> +  new_name = make_ssa_name (repl, stmt);
>  
>    if (dump_file)
>      {
>        fprintf (dump_file, "replacing an SSA name of a removed param ");
> -      print_generic_expr (dump_file, lhs, 0);
> +      print_generic_expr (dump_file, old_name, 0);
>        fprintf (dump_file, " with ");
> -      print_generic_expr (dump_file, name, 0);
> +      print_generic_expr (dump_file, new_name, 0);
>        fprintf (dump_file, "\n");
>      }
>  
> -  if (is_gimple_assign (stmt))
> -    gimple_assign_set_lhs (stmt, name);
> -  else if (is_gimple_call (stmt))
> -    gimple_call_set_lhs (stmt, name);
> -  else
> -    gimple_phi_set_result (as_a <gphi *> (stmt), name);
> -
> -  replace_uses_by (lhs, name);
> -  release_ssa_name (lhs);
> -  return true;
> +  replace_uses_by (old_name, new_name);
> +  return new_name;
>  }
>  
>  /* If the statement STMT contains any expressions that need to replaced with a
> @@ -4745,7 +4729,16 @@ ipa_sra_modify_function_body (ipa_parm_adjustment_vec adjustments)
>        gimple_stmt_iterator gsi;
>  
>        for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> -	replace_removed_params_ssa_names (gsi_stmt (gsi), adjustments);
> +	{
> +	  gphi *phi = as_a <gphi *> (gsi_stmt (gsi));
> +	  tree new_lhs, old_lhs = gimple_phi_result (phi);
> +	  new_lhs = replace_removed_params_ssa_names (old_lhs, phi, adjustments);
> +	  if (new_lhs)
> +	    {
> +	      gimple_phi_set_result (phi, new_lhs);
> +	      release_ssa_name (old_lhs);
> +	    }
> +	}
>  
>        gsi = gsi_start_bb (bb);
>        while (!gsi_end_p (gsi))
> @@ -4765,7 +4758,6 @@ ipa_sra_modify_function_body (ipa_parm_adjustment_vec adjustments)
>  
>  	    case GIMPLE_ASSIGN:
>  	      modified |= sra_ipa_modify_assign (stmt, &gsi, adjustments);
> -	      modified |= replace_removed_params_ssa_names (stmt, adjustments);
>  	      break;
>  
>  	    case GIMPLE_CALL:
> @@ -4780,8 +4772,6 @@ ipa_sra_modify_function_body (ipa_parm_adjustment_vec adjustments)
>  		{
>  		  t = gimple_call_lhs_ptr (stmt);
>  		  modified |= ipa_modify_expr (t, false, adjustments);
> -		  modified |= replace_removed_params_ssa_names (stmt,
> -								adjustments);
>  		}
>  	      break;
>  
> @@ -4805,6 +4795,20 @@ ipa_sra_modify_function_body (ipa_parm_adjustment_vec adjustments)
>  	      break;
>  	    }
>  
> +	  def_operand_p defp;
> +	  ssa_op_iter iter;
> +	  FOR_EACH_SSA_DEF_OPERAND (defp, stmt, iter, SSA_OP_DEF)
> +	    {
> +	      tree old_def = DEF_FROM_PTR (defp);
> +	      if (tree new_def = replace_removed_params_ssa_names (old_def, stmt,
> +								   adjustments))
> +		{
> +		  SET_DEF (defp, new_def);
> +		  release_ssa_name (old_def);
> +		  modified = true;
> +		}
> +	    }
> +
>  	  if (modified)
>  	    {
>  	      update_stmt (stmt);
> 
>

On 9 October 2015 at 09:46, Richard Biener <rguenther@suse.de> wrote:
> On Thu, 8 Oct 2015, Martin Jambor wrote:
>
>> Hi,
>>
>> the following fixes PR 67794 by properly remapping SSA_NAMEs which are
>> based on PARM_DECLs which are about to be removed as unnecessary.  And
>> by "properly" I mean also when they are defined by a GIMPL_ASM
>> statement.  In fact, it switches to using an iterator over definitions
>> to make sure it always handles everything...  well,except for PHIs
>> which are still handled specially because, from a quick glance over
>> their source, it seemed to me that the iterator does not support them.
>>
>> Bootstrapped and tested on x86_64-linux.  OK for trunk?
>> The issue is most probably latent on a number of old branches, do we
>> want to backport the patch to any of them?
>>
>> Thanks,
>>
>> Martin
>>
>>
>> 2015-10-08  Martin Jambor  <mjambor@suse.cz>
>>
>>       tree-optimization/67794
>>       * tree-sra.c (replace_removed_params_ssa_names): Do not distinguish
>>       between types of state,ents but accept original definitions as a
>>       parameter.
>>       (ipa_sra_modify_function_body): Use FOR_EACH_SSA_DEF_OPERAND to
>>       iterate over definitions.
>>
>> testsuite/
>>         * gcc.dg/ipa/ipa-sra-10.c: Nw test.
>>         * gcc.dg/torture/pr67794.c: Likewise.
>>
>> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-sra-10.c b/gcc/testsuite/gcc.dg/ipa/ipa-sra-10.c
>> new file mode 100644
>> index 0000000..24b64d1
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-sra-10.c
>> @@ -0,0 +1,34 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -fipa-sra -fdump-tree-eipa_sra-details"  } */
>> +
>> +extern void consume (int);
>> +extern int glob, glob1, glob2;
>> +extern int get (void);
>> +
>> +
>> +static void __attribute__ ((noinline))
>> +foo (int a)
>> +{
>> +  a = glob;
>> +  consume (a);
>> +  a = get ();
>> +  consume (a);
>> +  __asm__ volatile("" : : ""(a));
>> +  consume (a);
>> +
>> +  if (glob1)
>> +    a = glob1;
>> +  else
>> +    a = glob2;
>> +  consume (a);
>> +}
>> +
>> +int
>> +bar (int a)
>> +{
>> +  foo (a);
>> +  glob = a;
>> +  return 0;
>> +}
>> +
>> +/* { dg-final { scan-tree-dump-times "replacing an SSA name of a removed param" 4 "eipa_sra" } } */
>> diff --git a/gcc/testsuite/gcc.dg/torture/pr67794.c b/gcc/testsuite/gcc.dg/torture/pr67794.c
>> new file mode 100644
>> index 0000000..5489e56
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/torture/pr67794.c
>> @@ -0,0 +1,15 @@
>> +/* { dg-do compile } */
>> +
>> +int *b;
>> +static void fn1(int *best, int *dmin) {
>> +  int a[64];
>> +  dmin = a;
>> +  __asm__ volatile("" : "+&r"(dmin) : ""(best));
>> +}
>> +
>> +__attribute__((always_inline)) static inline void fn2(int *best) { fn1(best, b); }
>> +
>> +void fn3(void) {
>> +  int c[1];
>> +  fn2(c);
>> +}
>> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
>> index 4327990..f2a4e72 100644
>> --- a/gcc/tree-sra.c
>> +++ b/gcc/tree-sra.c
>> @@ -4612,61 +4612,45 @@ get_adjustment_for_base (ipa_parm_adjustment_vec adjustments, tree base)
>>    return NULL;
>>  }
>>
>> -/* If the statement STMT defines an SSA_NAME of a parameter which is to be
>> -   removed because its value is not used, replace the SSA_NAME with a one
>> -   relating to a created VAR_DECL together all of its uses and return true.
>> -   ADJUSTMENTS is a pointer to an adjustments vector.  */
>> +/* If OLD_NAME, which is being defined by statement STMT, is an SSA_NAME of a
>> +   parameter which is to be removed because its value is not used, create a new
>> +   SSA_NAME relating to a replacement VAR_DECL, replace all uses of the
>> +   original with it and return it.  If there is no need to re-map, return true.
>> +   ADJUSTMENTS is a pointer to a vector of IPA-SRA adjustments.  */
>
> The docs still say we return a bool
>
> Otherwise the patch looks ok to me.  I think we want to backport it
> to branche(s) after a while.
>
Hi Martin,

After your backport in the gcc-5 branch, I see build failures:
/tmp/2849532_27.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree-sra.c:
In function ‘tree_node* replace_removed_params_ssa_names(tree_node*,
gimple_statement_base**, ipa_parm_adjustment_vec)’:
/tmp/2849532_27.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree-sra.c:4609:
error: cannot convert ‘gimple_statement_base**’ to
‘gimple_statement_base*’ for argument ‘2’ to ‘tree_node*
make_ssa_name(tree_node*, gimple_statement_base*)’
/tmp/2849532_27.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree-sra.c:
In function ‘bool
ipa_sra_modify_function_body(ipa_parm_adjustment_vec)’:
/tmp/2849532_27.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree-sra.c:4703:
error: cannot convert ‘gphi*’ to ‘gimple_statement_base**’ for
argument ‘2’ to ‘tree_node*
replace_removed_params_ssa_names(tree_node*, gimple_statement_base**,
ipa_parm_adjustment_vec)’
/tmp/2849532_27.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/tree-sra.c:4772:
error: cannot convert ‘gimple_statement_base*’ to
‘gimple_statement_base**’ for argument ‘2’ to ‘tree_node*
replace_removed_params_ssa_names(tree_node*, gimple_statement_base**,
ipa_parm_adjustment_vec)’
make[2]: *** [tree-sra.o] Error 1

I see this on aarch64* and arm* targets.

Can you fix this?

Thanks,

Christophe.

> Thanks,
> Richard.
>
>> -static bool
>> -replace_removed_params_ssa_names (gimple *stmt,
>> +static tree
>> +replace_removed_params_ssa_names (tree old_name, gimple *stmt,
>>                                 ipa_parm_adjustment_vec adjustments)
>>  {
>>    struct ipa_parm_adjustment *adj;
>> -  tree lhs, decl, repl, name;
>> -
>> -  if (gimple_code (stmt) == GIMPLE_PHI)
>> -    lhs = gimple_phi_result (stmt);
>> -  else if (is_gimple_assign (stmt))
>> -    lhs = gimple_assign_lhs (stmt);
>> -  else if (is_gimple_call (stmt))
>> -    lhs = gimple_call_lhs (stmt);
>> -  else
>> -    gcc_unreachable ();
>> +  tree decl, repl, new_name;
>>
>> -  if (TREE_CODE (lhs) != SSA_NAME)
>> -    return false;
>> +  if (TREE_CODE (old_name) != SSA_NAME)
>> +    return NULL;
>>
>> -  decl = SSA_NAME_VAR (lhs);
>> +  decl = SSA_NAME_VAR (old_name);
>>    if (decl == NULL_TREE
>>        || TREE_CODE (decl) != PARM_DECL)
>> -    return false;
>> +    return NULL;
>>
>>    adj = get_adjustment_for_base (adjustments, decl);
>>    if (!adj)
>> -    return false;
>> +    return NULL;
>>
>>    repl = get_replaced_param_substitute (adj);
>> -  name = make_ssa_name (repl, stmt);
>> +  new_name = make_ssa_name (repl, stmt);
>>
>>    if (dump_file)
>>      {
>>        fprintf (dump_file, "replacing an SSA name of a removed param ");
>> -      print_generic_expr (dump_file, lhs, 0);
>> +      print_generic_expr (dump_file, old_name, 0);
>>        fprintf (dump_file, " with ");
>> -      print_generic_expr (dump_file, name, 0);
>> +      print_generic_expr (dump_file, new_name, 0);
>>        fprintf (dump_file, "\n");
>>      }
>>
>> -  if (is_gimple_assign (stmt))
>> -    gimple_assign_set_lhs (stmt, name);
>> -  else if (is_gimple_call (stmt))
>> -    gimple_call_set_lhs (stmt, name);
>> -  else
>> -    gimple_phi_set_result (as_a <gphi *> (stmt), name);
>> -
>> -  replace_uses_by (lhs, name);
>> -  release_ssa_name (lhs);
>> -  return true;
>> +  replace_uses_by (old_name, new_name);
>> +  return new_name;
>>  }
>>
>>  /* If the statement STMT contains any expressions that need to replaced with a
>> @@ -4745,7 +4729,16 @@ ipa_sra_modify_function_body (ipa_parm_adjustment_vec adjustments)
>>        gimple_stmt_iterator gsi;
>>
>>        for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>> -     replace_removed_params_ssa_names (gsi_stmt (gsi), adjustments);
>> +     {
>> +       gphi *phi = as_a <gphi *> (gsi_stmt (gsi));
>> +       tree new_lhs, old_lhs = gimple_phi_result (phi);
>> +       new_lhs = replace_removed_params_ssa_names (old_lhs, phi, adjustments);
>> +       if (new_lhs)
>> +         {
>> +           gimple_phi_set_result (phi, new_lhs);
>> +           release_ssa_name (old_lhs);
>> +         }
>> +     }
>>
>>        gsi = gsi_start_bb (bb);
>>        while (!gsi_end_p (gsi))
>> @@ -4765,7 +4758,6 @@ ipa_sra_modify_function_body (ipa_parm_adjustment_vec adjustments)
>>
>>           case GIMPLE_ASSIGN:
>>             modified |= sra_ipa_modify_assign (stmt, &gsi, adjustments);
>> -           modified |= replace_removed_params_ssa_names (stmt, adjustments);
>>             break;
>>
>>           case GIMPLE_CALL:
>> @@ -4780,8 +4772,6 @@ ipa_sra_modify_function_body (ipa_parm_adjustment_vec adjustments)
>>               {
>>                 t = gimple_call_lhs_ptr (stmt);
>>                 modified |= ipa_modify_expr (t, false, adjustments);
>> -               modified |= replace_removed_params_ssa_names (stmt,
>> -                                                             adjustments);
>>               }
>>             break;
>>
>> @@ -4805,6 +4795,20 @@ ipa_sra_modify_function_body (ipa_parm_adjustment_vec adjustments)
>>             break;
>>           }
>>
>> +       def_operand_p defp;
>> +       ssa_op_iter iter;
>> +       FOR_EACH_SSA_DEF_OPERAND (defp, stmt, iter, SSA_OP_DEF)
>> +         {
>> +           tree old_def = DEF_FROM_PTR (defp);
>> +           if (tree new_def = replace_removed_params_ssa_names (old_def, stmt,
>> +                                                                adjustments))
>> +             {
>> +               SET_DEF (defp, new_def);
>> +               release_ssa_name (old_def);
>> +               modified = true;
>> +             }
>> +         }
>> +
>>         if (modified)
>>           {
>>             update_stmt (stmt);
>>
>>
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)