Re: [PATCH, PR 42704] Do not remove scalar assignments in SRA

[Martin Jambor <mjambor at suse dot cz>]

Hi,

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?

Thanks,

Martin

2010-04-09  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.

Index: mine/gcc/tree-sra.c
===================================================================
*** mine.orig/gcc/tree-sra.c
--- mine/gcc/tree-sra.c
*************** sra_modify_constructor_assign (gimple *s
*** 2526,2531 ****
--- 2526,2562 ----
      }
  }
  
+ /* 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
*************** sra_modify_assign (gimple *stmt, gimple_
*** 2701,2726 ****
  	}
        else
  	{
! 	  if (access_has_children_p (racc))
  	    {
! 	      if (!racc->grp_unscalarized_data
! 		  /* Do not remove SSA name definitions (PR 42704).  */
! 		  && TREE_CODE (lhs) != SSA_NAME)
  		{
! 		  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_SA_REMOVED;
  		}
! 	      else
  		generate_subtree_copies (racc->first_child, lhs,
  					 racc->offset, 0, 0, gsi, false, true);
  	    }
! 	  else if (access_has_children_p (lacc))
  	    generate_subtree_copies (lacc->first_child, rhs, lacc->offset,
  				     0, 0, gsi, true, true);
  	}
--- 2732,2759 ----
  	}
        else
  	{
! 	  if (racc)
  	    {
! 	      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);
+ 
  		  unlink_stmt_vdef (*stmt);
  		  gsi_remove (gsi, true);
  		  sra_stats.deleted++;
  		  return SRA_SA_REMOVED;
  		}
! 	      else if (racc->first_child)
  		generate_subtree_copies (racc->first_child, lhs,
  					 racc->offset, 0, 0, gsi, false, true);
  	    }
! 	  if (access_has_children_p (lacc))
  	    generate_subtree_copies (lacc->first_child, rhs, lacc->offset,
  				     0, 0, gsi, true, true);
  	}