[PATCH] Fix 44891 - required type conversion in load elimination in SRA

[PATCH] Fix 44891 - required type conversion in load elimination in SRA

[Martin Jambor]

Hi,

with MEM_REF, the code in SRA that performs removal of loads from
pieces of aggregates that are known to be uninitialized can produce
statements with incompatible types when it creates a
default-definition replacement SSA_NAME for register loads.  

In order to try to keep the code complexity at least a bit sane, I
removed propagation of the LHS SSA_NAME with the replacement (fwprop
should do that just fine) and I no longer actually remove the
statement but rather simply adjust the RHS, potentially also
generating type conversion.

I have also added a simple message dump (and specifically want it to
be also in the non-detailed dumps) that tells a load is being removed
so that we can quickly see that this code path triggered when
analyzing SRA output.

I have bootstrapped and regression tested this patch on x86_64-linux
without any problems, OK for trunk?

Thanks,

Martin



2010-07-21  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/44891
	* tree-sra.c: Include gimple-pretty-print.h.
	(replace_uses_with_default_def_ssa_name): Renamed to
	get_repl_default_def_ssa_name, return the new SSA name instead of
	replacing the old one.
	(sra_modify_assign): Dump a message when removing a load, if the LHS
	is an SSA_NAME, do not do any propagation, just set the RHS to a
	default definition SSA NAME, type convert if necessary.
	* Makefile.in (tree-sra.o): Add gimple-pretty-print.h to dependencies.

	* testsuite/gcc.c-torture/compile/pr44891.c: New test.

Index: mine/gcc/tree-sra.c
===================================================================
--- mine.orig/gcc/tree-sra.c
+++ mine/gcc/tree-sra.c
@@ -90,6 +90,7 @@ along with GCC; see the file COPYING3.
 #include "flags.h"
 #include "dbgcnt.h"
 #include "tree-inline.h"
+#include "gimple-pretty-print.h"
 
 /* Enumeration of all aggregate reductions we can do.  */
 enum sra_mode { SRA_MODE_EARLY_IPA,   /* early call regularization */
@@ -2542,12 +2543,12 @@ sra_modify_constructor_assign (gimple *s
     }
 }
 
-/* Create a new suitable default definition SSA_NAME and replace all uses of
-   SSA with it, RACC is access describing the uninitialized part of an
-   aggregate that is being loaded.  */
+/* Create and return a new suitable default definition SSA_NAME for RACC which
+   is an access describing an uninitialized part of an aggregate that is being
+   loaded.  */
 
-static void
-replace_uses_with_default_def_ssa_name (tree ssa, struct access *racc)
+static tree
+get_repl_default_def_ssa_name (struct access *racc)
 {
   tree repl, decl;
 
@@ -2560,7 +2561,7 @@ replace_uses_with_default_def_ssa_name (
       set_default_def (decl, repl);
     }
 
-  replace_uses_by (ssa, repl);
+  return repl;
 }
 
 /* Examine both sides of the assignment statement pointed to by STMT, replace
@@ -2737,18 +2738,33 @@ sra_modify_assign (gimple *stmt, gimple_
 	    {
 	      if (!racc->grp_to_be_replaced && !racc->grp_unscalarized_data)
 		{
-		  if (racc->first_child)
-		    generate_subtree_copies (racc->first_child, lhs,
-					     racc->offset, 0, 0, gsi,
-					     false, false);
-		  gcc_assert (*stmt == gsi_stmt (*gsi));
-		  if (TREE_CODE (lhs) == SSA_NAME)
-		    replace_uses_with_default_def_ssa_name (lhs, racc);
+		  if (dump_file)
+		    {
+		      fprintf (dump_file, "Removing load: ");
+		      print_gimple_stmt (dump_file, *stmt, 0, 0);
+		    }
 
-		  unlink_stmt_vdef (*stmt);
-		  gsi_remove (gsi, true);
-		  sra_stats.deleted++;
-		  return SRA_AM_REMOVED;
+		  if (TREE_CODE (lhs) == SSA_NAME)
+		    {
+		      rhs = get_repl_default_def_ssa_name (racc);
+		      if (!useless_type_conversion_p (TREE_TYPE (lhs),
+						      TREE_TYPE (rhs)))
+			rhs = fold_build1_loc (loc, VIEW_CONVERT_EXPR,
+					       TREE_TYPE (lhs), rhs);
+		    }
+		  else
+		    {
+		      if (racc->first_child)
+			generate_subtree_copies (racc->first_child, lhs,
+						 racc->offset, 0, 0, gsi,
+						 false, false);
+
+		      gcc_assert (*stmt == gsi_stmt (*gsi));
+		      unlink_stmt_vdef (*stmt);
+		      gsi_remove (gsi, true);
+		      sra_stats.deleted++;
+		      return SRA_AM_REMOVED;
+		    }
 		}
 	      else if (racc->first_child)
 		generate_subtree_copies (racc->first_child, lhs,
Index: mine/gcc/Makefile.in
===================================================================
--- mine.orig/gcc/Makefile.in
+++ mine/gcc/Makefile.in
@@ -3132,7 +3132,7 @@ tree-sra.o : tree-sra.c $(CONFIG_H) $(SY
    $(TM_H) $(TREE_H) $(GIMPLE_H) $(CGRAPH_H) $(TREE_FLOW_H) $(IPA_PROP_H) \
    $(DIAGNOSTIC_H) statistics.h $(TREE_DUMP_H) $(TIMEVAR_H) $(PARAMS_H) \
    $(TARGET_H) $(FLAGS_H) $(EXPR_H) tree-pretty-print.h $(DBGCNT_H) \
-   $(TREE_INLINE_H)
+   $(TREE_INLINE_H) gimple-pretty-print.h
 tree-switch-conversion.o : tree-switch-conversion.c $(CONFIG_H) $(SYSTEM_H) \
     $(TREE_H) $(TM_P_H) $(TREE_FLOW_H) $(DIAGNOSTIC_H) $(TREE_INLINE_H) \
     $(TIMEVAR_H) $(TM_H) coretypes.h $(TREE_DUMP_H) $(GIMPLE_H) \
Index: mine/gcc/testsuite/gcc.c-torture/compile/pr44891.c
===================================================================
--- /dev/null
+++ mine/gcc/testsuite/gcc.c-torture/compile/pr44891.c
@@ -0,0 +1,26 @@
+struct S
+{
+  float f;
+  long l;
+};
+
+extern int gi;
+extern float gf;
+
+long foo (long p)
+{
+  struct S s;
+  float *pf;
+
+  s.l = p;
+
+  pf = &s.f;
+
+  pf++;
+  pf--;
+
+  gf = *pf + 3.3;
+  gi = *((int *)pf) + 2;
+
+  return s.l + 6;
+}

Re: [PATCH] Fix 44891 - required type conversion in load elimination in SRA

[Richard Guenther]

On Thu, 22 Jul 2010, Martin Jambor wrote:

> Hi,
> 
> with MEM_REF, the code in SRA that performs removal of loads from
> pieces of aggregates that are known to be uninitialized can produce
> statements with incompatible types when it creates a
> default-definition replacement SSA_NAME for register loads.  
> 
> In order to try to keep the code complexity at least a bit sane, I
> removed propagation of the LHS SSA_NAME with the replacement (fwprop
> should do that just fine) and I no longer actually remove the
> statement but rather simply adjust the RHS, potentially also
> generating type conversion.
> 
> I have also added a simple message dump (and specifically want it to
> be also in the non-detailed dumps) that tells a load is being removed
> so that we can quickly see that this code path triggered when
> analyzing SRA output.
> 
> I have bootstrapped and regression tested this patch on x86_64-linux
> without any problems, OK for trunk?

Ok.

Thanks,
Richard.

> Thanks,
> 
> Martin
> 
> 
> 
> 2010-07-21  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR tree-optimization/44891
> 	* tree-sra.c: Include gimple-pretty-print.h.
> 	(replace_uses_with_default_def_ssa_name): Renamed to
> 	get_repl_default_def_ssa_name, return the new SSA name instead of
> 	replacing the old one.
> 	(sra_modify_assign): Dump a message when removing a load, if the LHS
> 	is an SSA_NAME, do not do any propagation, just set the RHS to a
> 	default definition SSA NAME, type convert if necessary.
> 	* Makefile.in (tree-sra.o): Add gimple-pretty-print.h to dependencies.
> 
> 	* testsuite/gcc.c-torture/compile/pr44891.c: New test.
> 
> Index: mine/gcc/tree-sra.c
> ===================================================================
> --- mine.orig/gcc/tree-sra.c
> +++ mine/gcc/tree-sra.c
> @@ -90,6 +90,7 @@ along with GCC; see the file COPYING3.
>  #include "flags.h"
>  #include "dbgcnt.h"
>  #include "tree-inline.h"
> +#include "gimple-pretty-print.h"
>  
>  /* Enumeration of all aggregate reductions we can do.  */
>  enum sra_mode { SRA_MODE_EARLY_IPA,   /* early call regularization */
> @@ -2542,12 +2543,12 @@ sra_modify_constructor_assign (gimple *s
>      }
>  }
>  
> -/* Create a new suitable default definition SSA_NAME and replace all uses of
> -   SSA with it, RACC is access describing the uninitialized part of an
> -   aggregate that is being loaded.  */
> +/* Create and return a new suitable default definition SSA_NAME for RACC which
> +   is an access describing an uninitialized part of an aggregate that is being
> +   loaded.  */
>  
> -static void
> -replace_uses_with_default_def_ssa_name (tree ssa, struct access *racc)
> +static tree
> +get_repl_default_def_ssa_name (struct access *racc)
>  {
>    tree repl, decl;
>  
> @@ -2560,7 +2561,7 @@ replace_uses_with_default_def_ssa_name (
>        set_default_def (decl, repl);
>      }
>  
> -  replace_uses_by (ssa, repl);
> +  return repl;
>  }
>  
>  /* Examine both sides of the assignment statement pointed to by STMT, replace
> @@ -2737,18 +2738,33 @@ sra_modify_assign (gimple *stmt, gimple_
>  	    {
>  	      if (!racc->grp_to_be_replaced && !racc->grp_unscalarized_data)
>  		{
> -		  if (racc->first_child)
> -		    generate_subtree_copies (racc->first_child, lhs,
> -					     racc->offset, 0, 0, gsi,
> -					     false, false);
> -		  gcc_assert (*stmt == gsi_stmt (*gsi));
> -		  if (TREE_CODE (lhs) == SSA_NAME)
> -		    replace_uses_with_default_def_ssa_name (lhs, racc);
> +		  if (dump_file)
> +		    {
> +		      fprintf (dump_file, "Removing load: ");
> +		      print_gimple_stmt (dump_file, *stmt, 0, 0);
> +		    }
>  
> -		  unlink_stmt_vdef (*stmt);
> -		  gsi_remove (gsi, true);
> -		  sra_stats.deleted++;
> -		  return SRA_AM_REMOVED;
> +		  if (TREE_CODE (lhs) == SSA_NAME)
> +		    {
> +		      rhs = get_repl_default_def_ssa_name (racc);
> +		      if (!useless_type_conversion_p (TREE_TYPE (lhs),
> +						      TREE_TYPE (rhs)))
> +			rhs = fold_build1_loc (loc, VIEW_CONVERT_EXPR,
> +					       TREE_TYPE (lhs), rhs);
> +		    }
> +		  else
> +		    {
> +		      if (racc->first_child)
> +			generate_subtree_copies (racc->first_child, lhs,
> +						 racc->offset, 0, 0, gsi,
> +						 false, false);
> +
> +		      gcc_assert (*stmt == gsi_stmt (*gsi));
> +		      unlink_stmt_vdef (*stmt);
> +		      gsi_remove (gsi, true);
> +		      sra_stats.deleted++;
> +		      return SRA_AM_REMOVED;
> +		    }
>  		}
>  	      else if (racc->first_child)
>  		generate_subtree_copies (racc->first_child, lhs,
> Index: mine/gcc/Makefile.in
> ===================================================================
> --- mine.orig/gcc/Makefile.in
> +++ mine/gcc/Makefile.in
> @@ -3132,7 +3132,7 @@ tree-sra.o : tree-sra.c $(CONFIG_H) $(SY
>     $(TM_H) $(TREE_H) $(GIMPLE_H) $(CGRAPH_H) $(TREE_FLOW_H) $(IPA_PROP_H) \
>     $(DIAGNOSTIC_H) statistics.h $(TREE_DUMP_H) $(TIMEVAR_H) $(PARAMS_H) \
>     $(TARGET_H) $(FLAGS_H) $(EXPR_H) tree-pretty-print.h $(DBGCNT_H) \
> -   $(TREE_INLINE_H)
> +   $(TREE_INLINE_H) gimple-pretty-print.h
>  tree-switch-conversion.o : tree-switch-conversion.c $(CONFIG_H) $(SYSTEM_H) \
>      $(TREE_H) $(TM_P_H) $(TREE_FLOW_H) $(DIAGNOSTIC_H) $(TREE_INLINE_H) \
>      $(TIMEVAR_H) $(TM_H) coretypes.h $(TREE_DUMP_H) $(GIMPLE_H) \
> Index: mine/gcc/testsuite/gcc.c-torture/compile/pr44891.c
> ===================================================================
> --- /dev/null
> +++ mine/gcc/testsuite/gcc.c-torture/compile/pr44891.c
> @@ -0,0 +1,26 @@
> +struct S
> +{
> +  float f;
> +  long l;
> +};
> +
> +extern int gi;
> +extern float gf;
> +
> +long foo (long p)
> +{
> +  struct S s;
> +  float *pf;
> +
> +  s.l = p;
> +
> +  pf = &s.f;
> +
> +  pf++;
> +  pf--;
> +
> +  gf = *pf + 3.3;
> +  gi = *((int *)pf) + 2;
> +
> +  return s.l + 6;
> +}
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex

Re: [PATCH] Fix 44891 - required type conversion in load elimination in SRA

[H.J. Lu]

On Thu, Jul 22, 2010 at 3:44 AM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> with MEM_REF, the code in SRA that performs removal of loads from
> pieces of aggregates that are known to be uninitialized can produce
> statements with incompatible types when it creates a
> default-definition replacement SSA_NAME for register loads.
>
> In order to try to keep the code complexity at least a bit sane, I
> removed propagation of the LHS SSA_NAME with the replacement (fwprop
> should do that just fine) and I no longer actually remove the
> statement but rather simply adjust the RHS, potentially also
> generating type conversion.
>
> I have also added a simple message dump (and specifically want it to
> be also in the non-detailed dumps) that tells a load is being removed
> so that we can quickly see that this code path triggered when
> analyzing SRA output.
>
> I have bootstrapped and regression tested this patch on x86_64-linux
> without any problems, OK for trunk?
>
> Thanks,
>
> Martin
>
>
>
> 2010-07-21  Martin Jambor  <mjambor@suse.cz>
>
>        PR tree-optimization/44891
>        * tree-sra.c: Include gimple-pretty-print.h.
>        (replace_uses_with_default_def_ssa_name): Renamed to
>        get_repl_default_def_ssa_name, return the new SSA name instead of
>        replacing the old one.
>        (sra_modify_assign): Dump a message when removing a load, if the LHS
>        is an SSA_NAME, do not do any propagation, just set the RHS to a
>        default definition SSA NAME, type convert if necessary.
>        * Makefile.in (tree-sra.o): Add gimple-pretty-print.h to dependencies.
>
>        * testsuite/gcc.c-torture/compile/pr44891.c: New test.
>

This caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45415


-- 
H.J.