[PATCH, PR 42704] Do not remove scalar assignments in SRA
Martin Jambor
mjambor@suse.cz
Tue Apr 13 13:48:00 GMT 2010
Hi,
On Mon, Apr 12, 2010 at 11:48:12AM +0200, Richard Guenther wrote:
> On Fri, 9 Apr 2010, Martin Jambor wrote:
> >
> > On Fri, Jan 15, 2010 at 01:09:13PM +0100, Martin Jambor wrote:
> > > On Wed, Jan 13, 2010 at 04:54:12PM +0100, Richard Guenther wrote:
> > > > On Wed, 13 Jan 2010, Martin Jambor wrote:
> > > > >
> > > > > SRA sometimes removes assignment statements when it knows that on the
> > > > > RHS all live data (ranges that have stores to) have scalar
> > > > > replacements. This caused a verification issue PR 42704 when a
> > > > > removal of a scalar load from uninitialized memory (and therefore
> > > > > without any live data) resulted in an SSA_NAME without a definition.
> > > > > (This actually happened in in dead code.)
> > > > >
> > > > > This happens only in very rare cases (scalar accesses with
> > > > > sub-accesses) and the statements are most likely to be eliminated
> > > > > later on anyway so I believe the solution is simply not to do this if
> > > > > the type of the assignment is is_gimple_reg_type.
> > > > >
> > > > > Bootstrapped and tested on x86_64-linux. OK for trunk?
> > > >
> > > > Hm, either never remove stmts with a SSA name lhs or propagate
> > > > the default defintion (if not a parameter! in that case create
> > > > a new SSA name) into all uses.
> > > >
> > > > Btw, I wonder why
> > > >
> > > > D.2215_9 = D.2163.mLink[i_8].mParent;
> > > >
> > > > is removed but
> > > >
> > > > D.2216_10 = D.2163.mLink[i_8].mChildIndex;
> > > >
> > > > is not. Looks like a missed optimization.
> > > >
> > > > The patch is ok for 4.5 if you replace
> > > >
> > > > > + /* When this is a register-type assignment, the LHS is
> > > > an
> > > > > + SSA_NAME definition that we have to keep even when
> > > > loading
> > > > > + uninitialized data (PR 42704). */
> > > > > + && !is_gimple_reg_type (TREE_TYPE (lhs)))
> > > >
> > > > with
> > > >
> > > > /* Do not remove SSA name definitions. */
> > > > && TREE_CODE (lhs) != SSA_NAME)
> > > >
> > >
> > > OK, I did that.
> > >
> > > > and file a missed-optimization bugreport about this (the read from
> > > > uninitialized memory keeps the memory life - I think nothing else
> > > > optimizes this).
> > > >
> > >
> > > The patch below performs both the substitutions and removal of loads
> > > of uninitialized data even when there is no subaccess to the
> > > corresponding access. I t bootstraps and tests fine, I have added it
> > > to the patches to submit in stage1.
> > >
> >
> > Since we now are in stage1, I'd like to commit a slightly amended
> > version below. Bootstrapped and tested on x86-64-linux with no
> > regressions and I have verified it removes the statement in the
> > testcase that we wanted to be gone. OK for trunk?
>
> Can you add a testcase for this? Ok with that change.
OK, this is what I committed as revision 158271.
Thanks,
Martin
2010-04-13 Martin Jambor <mjambor@suse.cz>
* tree-sra.c (replace_uses_with_default_def_ssa_name): New function.
(sra_modify_assign): Delete stmts loading dead data even if racc has no
children. Call replace_uses_with_default_def_ssa_name to handle
SSA_NAES on lhs.
* testsuite/gcc.dg/tree-ssa/sra-9.c: New test.
Index: mine/gcc/tree-sra.c
===================================================================
--- mine.orig/gcc/tree-sra.c
+++ mine/gcc/tree-sra.c
@@ -2528,6 +2528,37 @@ sra_modify_constructor_assign (gimple *s
}
}
+/* Create a new suitable default definition SSA_NAME and replace all uses of
+ SSA with it. */
+
+static void
+replace_uses_with_default_def_ssa_name (tree ssa)
+{
+ tree repl, decl = SSA_NAME_VAR (ssa);
+ if (TREE_CODE (decl) == PARM_DECL)
+ {
+ tree tmp = create_tmp_var (TREE_TYPE (decl), "SR");
+ if (TREE_CODE (TREE_TYPE (tmp)) == COMPLEX_TYPE
+ || TREE_CODE (TREE_TYPE (tmp)) == VECTOR_TYPE)
+ DECL_GIMPLE_REG_P (tmp) = 1;
+
+ get_var_ann (tmp);
+ add_referenced_var (tmp);
+ repl = make_ssa_name (tmp, gimple_build_nop ());
+ set_default_def (tmp, repl);
+ }
+ else
+ {
+ repl = gimple_default_def (cfun, decl);
+ if (!repl)
+ {
+ repl = make_ssa_name (decl, gimple_build_nop ());
+ set_default_def (decl, repl);
+ }
+ }
+
+ replace_uses_by (ssa, repl);
+}
/* Callback of scan_function to process assign statements. It examines both
sides of the statement, replaces them with a scalare replacement if there is
@@ -2703,26 +2734,28 @@ sra_modify_assign (gimple *stmt, gimple_
}
else
{
- if (access_has_children_p (racc))
+ if (racc)
{
- if (!racc->grp_unscalarized_data
- /* Do not remove SSA name definitions (PR 42704). */
- && TREE_CODE (lhs) != SSA_NAME)
+ if (!racc->grp_to_be_replaced && !racc->grp_unscalarized_data)
{
- generate_subtree_copies (racc->first_child, lhs,
- racc->offset, 0, 0, gsi,
- false, false);
+ 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);
+
unlink_stmt_vdef (*stmt);
gsi_remove (gsi, true);
sra_stats.deleted++;
return SRA_SA_REMOVED;
}
- else
+ else if (racc->first_child)
generate_subtree_copies (racc->first_child, lhs,
racc->offset, 0, 0, gsi, false, true);
}
- else if (access_has_children_p (lacc))
+ if (access_has_children_p (lacc))
generate_subtree_copies (lacc->first_child, rhs, lacc->offset,
0, 0, gsi, true, true);
}
Index: mine/gcc/testsuite/gcc.dg/tree-ssa/sra-9.c
===================================================================
--- /dev/null
+++ mine/gcc/testsuite/gcc.dg/tree-ssa/sra-9.c
@@ -0,0 +1,20 @@
+/* { dg-do compile } */
+/* { dg-options "-O1 -fdump-tree-optimized" } */
+
+struct S
+{
+ int a, b, c;
+ int z[20];
+};
+
+int foo (int d)
+{
+ struct S s;
+
+ s.a = d;
+ return s.a + s.b;
+}
+
+/* There should be no reference to s.b. */
+/* { dg-final { scan-tree-dump-times "s\.b" 0 "optimized"} } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
More information about the Gcc-patches
mailing list