| Message ID | 20100722104408.GB8513@virgil.arch.suse.de |
|---|---|
| State | New |
| Headers | show |
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; > +} > >
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
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; +}