update address taken: don't drop clobbers

update address taken: don't drop clobbers

[Marc Glisse]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1560 bytes --]

Hello,

we currently drop clobbers on variables whose address is not taken 
anymore. However, rewrite_stmt has code to replace them with an SSA_NAME 
with a default definition (an uninitialized variable), and I believe 
rewrite_update_stmt should do the same. This allows us to warn sometimes 
(see testcase), but during the debugging I also noticed several places 
where it allowed CCP to simplify further PHIs, so this is also an 
optimization.

In an earlier version of the patch, I was using
get_or_create_ssa_default_def (cfun, sym);
(I was reusing the same variable). This passed bootstrap+testsuite on all 
languages except for ada. Indeed, the compiler wanted to coalesce several 
SSA_NAMEs, including those new ones, in out-of-ssa, but couldn't. There 
are abnormal PHIs involved. Maybe it shouldn't have insisted on coalescing 
an undefined ssa_name, maybe something should have prevented us from 
reaching such a situation, but creating a new variable was the simplest 
workaround.

Some things could be done to improve the error message in uninit:
- getting the location of the variable,
- differenciating uninitialized from clobbered,
but that can come later.

Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-unknown-linux-gnu.

2014-06-30  Marc Glisse  <marc.glisse@inria.fr>

 	PR tree-optimization/60770
gcc/
 	* tree-ssa.c (execute_update_addresses_taken): Don't drop clobbers.
 	* tree-into-ssa.c (maybe_register_def): Replace clobbers with a
 	default definition.
gcc/testsuite/
 	* gcc.dg/tree-ssa/pr60770-1.c: New file.

-- 
Marc Glisse

[-- Attachment #2: Type: TEXT/PLAIN, Size: 3366 bytes --]

Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c	(working copy)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O -Wall" } */
+
+int f(int n){
+  int*p;
+  {
+    int yyy=n;
+    p=&yyy;
+  }
+  return *p; /* { dg-warning "yyy" } */
+}
Index: gcc/tree-into-ssa.c
===================================================================
--- gcc/tree-into-ssa.c	(revision 212109)
+++ gcc/tree-into-ssa.c	(working copy)
@@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
 {
   tree def = DEF_FROM_PTR (def_p);
   tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
 
   /* If DEF is a naked symbol that needs renaming, create a new
      name for it.  */
   if (marked_for_renaming (sym))
     {
       if (DECL_P (def))
 	{
-	  tree tracked_var;
-
-	  def = make_ssa_name (def, stmt);
+	  if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
+	    {
+	      /* Replace clobber stmts with a default def.  Create a new
+		 variable so we don't later think we must coalesce, which would
+		 fail with some ada abnormal PHIs.  Still, we try to keep a
+		 similar name so error messages make sense.  */
+	      unlink_stmt_vdef (stmt);
+	      gsi_replace (&gsi, gimple_build_nop (), true);
+	      tree id = DECL_NAME (sym);
+	      const char* name = id ? IDENTIFIER_POINTER (id) : 0;
+	      tree newvar = create_tmp_var (TREE_TYPE (sym), name);
+	      def = get_or_create_ssa_default_def (cfun, newvar);
+	    }
+	  else
+	    def = make_ssa_name (def, stmt);
 	  SET_DEF (def_p, def);
 
-	  tracked_var = target_for_debug_bind (sym);
+	  tree tracked_var = target_for_debug_bind (sym);
 	  if (tracked_var)
 	    {
 	      gimple note = gimple_build_debug_bind (tracked_var, def, stmt);
 	      /* If stmt ends the bb, insert the debug stmt on the single
 		 non-EH edge from the stmt.  */
 	      if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
 		{
 		  basic_block bb = gsi_bb (gsi);
 		  edge_iterator ei;
 		  edge e, ef = NULL;
Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c	(revision 212109)
+++ gcc/tree-ssa.c	(working copy)
@@ -1607,32 +1607,20 @@ execute_update_addresses_taken (void)
 		rhs = gimple_assign_rhs1 (stmt);
 		if (gimple_assign_lhs (stmt) != lhs
 		    && !useless_type_conversion_p (TREE_TYPE (lhs),
 						   TREE_TYPE (rhs)))
 		  rhs = fold_build1 (VIEW_CONVERT_EXPR,
 				     TREE_TYPE (lhs), rhs);
 
 		if (gimple_assign_lhs (stmt) != lhs)
 		  gimple_assign_set_lhs (stmt, lhs);
 
-		/* For var ={v} {CLOBBER}; where var lost
-		   TREE_ADDRESSABLE just remove the stmt.  */
-		if (DECL_P (lhs)
-		    && TREE_CLOBBER_P (rhs)
-		    && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
-		  {
-		    unlink_stmt_vdef (stmt);
-      		    gsi_remove (&gsi, true);
-		    release_defs (stmt);
-		    continue;
-		  }
-
 		if (gimple_assign_rhs1 (stmt) != rhs)
 		  {
 		    gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
 		    gimple_assign_set_rhs_from_tree (&gsi, rhs);
 		  }
 	      }
 
 	    else if (gimple_code (stmt) == GIMPLE_CALL)
 	      {
 		unsigned i;

SRA: don't drop clobbers

[Marc Glisse]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1029 bytes --]

Hello,

with this patch on top of
https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02315.html
we finally warn for the testcase of PR 60517.

The new function is copied from init_subtree_with_zero right above. I 
guess it might be possible to merge them into a single function, if 
desired. I don't understand the debug stuff, but hopefully by keeping the 
functions similar enough it shouldn't be too broken.

When we see a clobber during scalarization, instead of dropping it, we add 
a clobber to the new variable, which the previous patch turns into an 
SSA_NAME with a default def. Then either we reach uninit and warn, or the 
variable appears in a PHI and CCP optimizes.

Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-linux-gnu.


2014-07-01  Marc Glisse  <marc.glisse@inria.fr>

         PR c++/60517
         PR tree-optimization/60770
gcc/
         * tree-sra.c (clobber_subtree): New function.
 	(sra_modify_constructor_assign): Call it.
gcc/testsuite/
         * g++.dg/tree-ssa/pr60517.C: New file.

-- 
Marc Glisse

[-- Attachment #2: Type: TEXT/PLAIN, Size: 4257 bytes --]

Index: gcc/tree-sra.c
===================================================================
--- gcc/tree-sra.c	(revision 212126)
+++ gcc/tree-sra.c	(working copy)
@@ -2727,20 +2727,58 @@ init_subtree_with_zero (struct access *a
       if (insert_after)
 	gsi_insert_after (gsi, ds, GSI_NEW_STMT);
       else
 	gsi_insert_before (gsi, ds, GSI_SAME_STMT);
     }
 
   for (child = access->first_child; child; child = child->next_sibling)
     init_subtree_with_zero (child, gsi, insert_after, loc);
 }
 
+static void
+clobber_subtree (struct access *access, gimple_stmt_iterator *gsi,
+		bool insert_after, location_t loc)
+
+{
+  struct access *child;
+
+  if (access->grp_to_be_replaced)
+    {
+      tree rep = get_access_replacement (access);
+      tree clobber = build_constructor (access->type, NULL);
+      TREE_THIS_VOLATILE (clobber) = 1;
+      gimple stmt = gimple_build_assign (rep, clobber);
+
+      if (insert_after)
+	gsi_insert_after (gsi, stmt, GSI_NEW_STMT);
+      else
+	gsi_insert_before (gsi, stmt, GSI_SAME_STMT);
+      update_stmt (stmt);
+      gimple_set_location (stmt, loc);
+    }
+  else if (access->grp_to_be_debug_replaced)
+    {
+      tree rep = get_access_replacement (access);
+      tree clobber = build_constructor (access->type, NULL);
+      TREE_THIS_VOLATILE (clobber) = 1;
+      gimple ds = gimple_build_debug_bind (rep, clobber, gsi_stmt (*gsi));
+
+      if (insert_after)
+	gsi_insert_after (gsi, ds, GSI_NEW_STMT);
+      else
+	gsi_insert_before (gsi, ds, GSI_SAME_STMT);
+    }
+
+  for (child = access->first_child; child; child = child->next_sibling)
+    clobber_subtree (child, gsi, insert_after, loc);
+}
+
 /* Search for an access representative for the given expression EXPR and
    return it or NULL if it cannot be found.  */
 
 static struct access *
 get_access_for_expr (tree expr)
 {
   HOST_WIDE_INT offset, size, max_size;
   tree base;
 
   /* FIXME: This should not be necessary but Ada produces V_C_Es with a type of
@@ -3039,43 +3077,41 @@ enum assignment_mod_result { SRA_AM_NONE
 			     SRA_AM_REMOVED };  /* stmt eliminated */
 
 /* Modify assignments with a CONSTRUCTOR on their RHS.  STMT contains a pointer
    to the assignment and GSI is the statement iterator pointing at it.  Returns
    the same values as sra_modify_assign.  */
 
 static enum assignment_mod_result
 sra_modify_constructor_assign (gimple *stmt, gimple_stmt_iterator *gsi)
 {
   tree lhs = gimple_assign_lhs (*stmt);
-  struct access *acc;
-  location_t loc;
-
-  acc = get_access_for_expr (lhs);
+  struct access *acc = get_access_for_expr (lhs);
   if (!acc)
     return SRA_AM_NONE;
+  location_t loc = gimple_location (*stmt);
 
   if (gimple_clobber_p (*stmt))
     {
-      /* Remove clobbers of fully scalarized variables, otherwise
-	 do nothing.  */
+      /* Clobber the replacement variable.  */
+      clobber_subtree (acc, gsi, !acc->grp_covered, loc);
+      /* Remove clobbers of fully scalarized variables, they are dead.  */
       if (acc->grp_covered)
 	{
 	  unlink_stmt_vdef (*stmt);
 	  gsi_remove (gsi, true);
 	  release_defs (*stmt);
 	  return SRA_AM_REMOVED;
 	}
       else
-	return SRA_AM_NONE;
+	return SRA_AM_MODIFIED;
     }
 
-  loc = gimple_location (*stmt);
   if (vec_safe_length (CONSTRUCTOR_ELTS (gimple_assign_rhs1 (*stmt))) > 0)
     {
       /* I have never seen this code path trigger but if it can happen the
 	 following should handle it gracefully.  */
       if (access_has_children_p (acc))
 	generate_subtree_copies (acc->first_child, lhs, acc->offset, 0, 0, gsi,
 				 true, true, loc);
       return SRA_AM_MODIFIED;
     }
 
Index: gcc/testsuite/g++.dg/tree-ssa/pr60517.C
===================================================================
--- gcc/testsuite/g++.dg/tree-ssa/pr60517.C	(revision 0)
+++ gcc/testsuite/g++.dg/tree-ssa/pr60517.C	(working copy)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O -Wall" } */
+
+struct B {
+    double x[2];
+};
+struct A {
+    B b;
+    B getB() { return b; }
+};
+
+double foo(A a) {
+    double * x = &(a.getB().x[0]);
+    return x[0]; /* { dg-warning "" } */
+}

Re: SRA: don't drop clobbers

[Richard Biener]

On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Hello,
>
> with this patch on top of
> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02315.html
> we finally warn for the testcase of PR 60517.
>
> The new function is copied from init_subtree_with_zero right above. I guess
> it might be possible to merge them into a single function, if desired. I
> don't understand the debug stuff, but hopefully by keeping the functions
> similar enough it shouldn't be too broken.
>
> When we see a clobber during scalarization, instead of dropping it, we add a
> clobber to the new variable, which the previous patch turns into an SSA_NAME
> with a default def. Then either we reach uninit and warn, or the variable
> appears in a PHI and CCP optimizes.

What's the point of a clobber of sth that was scalarized away?  So ...
can you please explain in more detail?

> Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-linux-gnu.

The new function needs a comment.

Richard.

>
> 2014-07-01  Marc Glisse  <marc.glisse@inria.fr>
>
>         PR c++/60517
>         PR tree-optimization/60770
> gcc/
>         * tree-sra.c (clobber_subtree): New function.
>         (sra_modify_constructor_assign): Call it.
> gcc/testsuite/
>         * g++.dg/tree-ssa/pr60517.C: New file.
>
> --
> Marc Glisse
> Index: gcc/tree-sra.c
> ===================================================================
> --- gcc/tree-sra.c      (revision 212126)
> +++ gcc/tree-sra.c      (working copy)
> @@ -2727,20 +2727,58 @@ init_subtree_with_zero (struct access *a
>        if (insert_after)
>         gsi_insert_after (gsi, ds, GSI_NEW_STMT);
>        else
>         gsi_insert_before (gsi, ds, GSI_SAME_STMT);
>      }
>
>    for (child = access->first_child; child; child = child->next_sibling)
>      init_subtree_with_zero (child, gsi, insert_after, loc);
>  }
>
> +static void
> +clobber_subtree (struct access *access, gimple_stmt_iterator *gsi,
> +               bool insert_after, location_t loc)
> +
> +{
> +  struct access *child;
> +
> +  if (access->grp_to_be_replaced)
> +    {
> +      tree rep = get_access_replacement (access);
> +      tree clobber = build_constructor (access->type, NULL);
> +      TREE_THIS_VOLATILE (clobber) = 1;
> +      gimple stmt = gimple_build_assign (rep, clobber);
> +
> +      if (insert_after)
> +       gsi_insert_after (gsi, stmt, GSI_NEW_STMT);
> +      else
> +       gsi_insert_before (gsi, stmt, GSI_SAME_STMT);
> +      update_stmt (stmt);
> +      gimple_set_location (stmt, loc);
> +    }
> +  else if (access->grp_to_be_debug_replaced)
> +    {
> +      tree rep = get_access_replacement (access);
> +      tree clobber = build_constructor (access->type, NULL);
> +      TREE_THIS_VOLATILE (clobber) = 1;
> +      gimple ds = gimple_build_debug_bind (rep, clobber, gsi_stmt (*gsi));
> +
> +      if (insert_after)
> +       gsi_insert_after (gsi, ds, GSI_NEW_STMT);
> +      else
> +       gsi_insert_before (gsi, ds, GSI_SAME_STMT);
> +    }
> +
> +  for (child = access->first_child; child; child = child->next_sibling)
> +    clobber_subtree (child, gsi, insert_after, loc);
> +}
> +
>  /* Search for an access representative for the given expression EXPR and
>     return it or NULL if it cannot be found.  */
>
>  static struct access *
>  get_access_for_expr (tree expr)
>  {
>    HOST_WIDE_INT offset, size, max_size;
>    tree base;
>
>    /* FIXME: This should not be necessary but Ada produces V_C_Es with a
> type of
> @@ -3039,43 +3077,41 @@ enum assignment_mod_result { SRA_AM_NONE
>                              SRA_AM_REMOVED };  /* stmt eliminated */
>
>  /* Modify assignments with a CONSTRUCTOR on their RHS.  STMT contains a
> pointer
>     to the assignment and GSI is the statement iterator pointing at it.
> Returns
>     the same values as sra_modify_assign.  */
>
>  static enum assignment_mod_result
>  sra_modify_constructor_assign (gimple *stmt, gimple_stmt_iterator *gsi)
>  {
>    tree lhs = gimple_assign_lhs (*stmt);
> -  struct access *acc;
> -  location_t loc;
> -
> -  acc = get_access_for_expr (lhs);
> +  struct access *acc = get_access_for_expr (lhs);
>    if (!acc)
>      return SRA_AM_NONE;
> +  location_t loc = gimple_location (*stmt);
>
>    if (gimple_clobber_p (*stmt))
>      {
> -      /* Remove clobbers of fully scalarized variables, otherwise
> -        do nothing.  */
> +      /* Clobber the replacement variable.  */
> +      clobber_subtree (acc, gsi, !acc->grp_covered, loc);
> +      /* Remove clobbers of fully scalarized variables, they are dead.  */
>        if (acc->grp_covered)
>         {
>           unlink_stmt_vdef (*stmt);
>           gsi_remove (gsi, true);
>           release_defs (*stmt);
>           return SRA_AM_REMOVED;
>         }
>        else
> -       return SRA_AM_NONE;
> +       return SRA_AM_MODIFIED;
>      }
>
> -  loc = gimple_location (*stmt);
>    if (vec_safe_length (CONSTRUCTOR_ELTS (gimple_assign_rhs1 (*stmt))) > 0)
>      {
>        /* I have never seen this code path trigger but if it can happen the
>          following should handle it gracefully.  */
>        if (access_has_children_p (acc))
>         generate_subtree_copies (acc->first_child, lhs, acc->offset, 0, 0,
> gsi,
>                                  true, true, loc);
>        return SRA_AM_MODIFIED;
>      }
>
> Index: gcc/testsuite/g++.dg/tree-ssa/pr60517.C
> ===================================================================
> --- gcc/testsuite/g++.dg/tree-ssa/pr60517.C     (revision 0)
> +++ gcc/testsuite/g++.dg/tree-ssa/pr60517.C     (working copy)
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -Wall" } */
> +
> +struct B {
> +    double x[2];
> +};
> +struct A {
> +    B b;
> +    B getB() { return b; }
> +};
> +
> +double foo(A a) {
> +    double * x = &(a.getB().x[0]);
> +    return x[0]; /* { dg-warning "" } */
> +}
>

Re: SRA: don't drop clobbers

[Marc Glisse]

On Mon, 7 Jul 2014, Richard Biener wrote:

> On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> Hello,
>>
>> with this patch on top of
>> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02315.html
>> we finally warn for the testcase of PR 60517.
>>
>> The new function is copied from init_subtree_with_zero right above. I guess
>> it might be possible to merge them into a single function, if desired. I
>> don't understand the debug stuff, but hopefully by keeping the functions
>> similar enough it shouldn't be too broken.
>>
>> When we see a clobber during scalarization, instead of dropping it, we add a
>> clobber to the new variable, which the previous patch turns into an SSA_NAME
>> with a default def. Then either we reach uninit and warn, or the variable
>> appears in a PHI and CCP optimizes.
>
> What's the point of a clobber of sth that was scalarized away?  So ...
> can you please explain in more detail?

The main idea of these patches is that when we read from a place that was 
clobbered, instead of dropping the clobber and reading what was there 
before, we can use a variable with a default definition to mark that the 
content is undefined. This enables both warnings and optimizations.

The previous patch makes update_ssa handle replacing clobbers with default 
definitions when a variable doesn't have its address taken anymore. When 
SRA scalarizes, it creates a new variable and relies on update_ssa to 
finish the job. So I am inserting a clobber on the new variable so that 
update_ssa knows to use a default definition.

> The new function needs a comment.

Indeed, I'll add one once the conversation on the first patch converges.

Thanks,

-- 
Marc Glisse

Re: SRA: don't drop clobbers

[Richard Biener]

On July 7, 2014 11:32:10 AM CEST, Marc Glisse <marc.glisse@inria.fr> wrote:
>On Mon, 7 Jul 2014, Richard Biener wrote:
>
>> On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse <marc.glisse@inria.fr>
>wrote:
>>> Hello,
>>>
>>> with this patch on top of
>>> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02315.html
>>> we finally warn for the testcase of PR 60517.
>>>
>>> The new function is copied from init_subtree_with_zero right above.
>I guess
>>> it might be possible to merge them into a single function, if
>desired. I
>>> don't understand the debug stuff, but hopefully by keeping the
>functions
>>> similar enough it shouldn't be too broken.
>>>
>>> When we see a clobber during scalarization, instead of dropping it,
>we add a
>>> clobber to the new variable, which the previous patch turns into an
>SSA_NAME
>>> with a default def. Then either we reach uninit and warn, or the
>variable
>>> appears in a PHI and CCP optimizes.
>>
>> What's the point of a clobber of sth that was scalarized away?  So
>...
>> can you please explain in more detail?
>
>The main idea of these patches is that when we read from a place that
>was 
>clobbered, instead of dropping the clobber and reading what was there 
>before, we can use a variable with a default definition to mark that
>the 
>content is undefined. This enables both warnings and optimizations.
>
>The previous patch makes update_ssa handle replacing clobbers with
>default 
>definitions when a variable doesn't have its address taken anymore.
>When 
>SRA scalarizes, it creates a new variable and relies on update_ssa to 
>finish the job. So I am inserting a clobber on the new variable so that
>
>update_ssa knows to use a default definition.

OK.  But can't SRA simply replace the reads with undef SSA names when the use is dominated by a clobber?

(I've long wanted to teach it some flow sensitivity by doing the replacements within a domwalk)

Richard.

>> The new function needs a comment.
>
>Indeed, I'll add one once the conversation on the first patch
>converges.
>
>Thanks,

Re: SRA: don't drop clobbers

[Marc Glisse]

On Mon, 7 Jul 2014, Richard Biener wrote:

>> The main idea of these patches is that when we read from a place that 
>> was clobbered, instead of dropping the clobber and reading what was 
>> there before, we can use a variable with a default definition to mark 
>> that the content is undefined. This enables both warnings and 
>> optimizations.
>>
>> The previous patch makes update_ssa handle replacing clobbers with 
>> default definitions when a variable doesn't have its address taken 
>> anymore. When SRA scalarizes, it creates a new variable and relies on 
>> update_ssa to finish the job. So I am inserting a clobber on the new 
>> variable so that update_ssa knows to use a default definition.
>
> OK.  But can't SRA simply replace the reads with undef SSA names when 
> the use is dominated by a clobber?

I guess it could, but it seems safer and much simpler to avoid duplicating 
the logic.

SRA creates a new variable and replaces uses of the old one by uses of the 
new one. update_address_taken then handles replacing the new variable by a 
bunch of SSA_NAMEs, tracking which use corresponds to which def, so it 
seems natural to let it handle the clobber as well. Also, conceptually, 
saying that when the full variable is clobbered the subvariable is 
clobbered as well is a rather obvious operation.

Otherwise, I could create an extra variable tmp, with the same type as the 
new variable, and replace the clobber by an assignment of the default 
definition of tmp to the new variable. update_address_taken will then see 
a regular assignment and handle it as usual (create a new SSA_NAME for 
it). So instead of creating a clobber that is immediatly replaced with a 
NOP, this creates an extra SSA_NAME that only disappears in the next 
propagation pass.

Handling the reads myself directly in SRA and replacing them with undef 
SSA_NAMEs doesn't seem to fit well with how SRA works. It would certainly 
be possible, but would require quite a bit of new code. Also, the 
domination relation between the clobber and the reads is not always 
obvious and may require some PHIs in between.

-- 
Marc Glisse

Re: SRA: don't drop clobbers

[Jeff Law]

On 07/07/14 02:56, Richard Biener wrote:
> On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> Hello,
>>
>> with this patch on top of
>> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02315.html
>> we finally warn for the testcase of PR 60517.
>>
>> The new function is copied from init_subtree_with_zero right above. I guess
>> it might be possible to merge them into a single function, if desired. I
>> don't understand the debug stuff, but hopefully by keeping the functions
>> similar enough it shouldn't be too broken.
>>
>> When we see a clobber during scalarization, instead of dropping it, we add a
>> clobber to the new variable, which the previous patch turns into an SSA_NAME
>> with a default def. Then either we reach uninit and warn, or the variable
>> appears in a PHI and CCP optimizes.
>
> What's the point of a clobber of sth that was scalarized away?  So ...
> can you please explain in more detail?
Marc is trying to exploit the "undefinedness" of the object that exists 
after the CLOBBER.  When we scalaraize, we are losing the undefinedness 
quality of the object.

It's very similar to what's already happening in tree-into-ssa, he's 
just handling it in another place.

Jeff

Re: SRA: don't drop clobbers

[Richard Biener]

On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Hello,
>
> with this patch on top of
> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02315.html
> we finally warn for the testcase of PR 60517.
>
> The new function is copied from init_subtree_with_zero right above. I guess
> it might be possible to merge them into a single function, if desired. I
> don't understand the debug stuff, but hopefully by keeping the functions
> similar enough it shouldn't be too broken.
>
> When we see a clobber during scalarization, instead of dropping it, we add a
> clobber to the new variable, which the previous patch turns into an SSA_NAME
> with a default def. Then either we reach uninit and warn, or the variable
> appears in a PHI and CCP optimizes.
>
> Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-linux-gnu.
>
>
> 2014-07-01  Marc Glisse  <marc.glisse@inria.fr>
>
>         PR c++/60517
>         PR tree-optimization/60770
> gcc/
>         * tree-sra.c (clobber_subtree): New function.
>         (sra_modify_constructor_assign): Call it.
> gcc/testsuite/
>         * g++.dg/tree-ssa/pr60517.C: New file.
>
> --
> Marc Glisse
> Index: gcc/tree-sra.c
> ===================================================================
> --- gcc/tree-sra.c      (revision 212126)
> +++ gcc/tree-sra.c      (working copy)
> @@ -2727,20 +2727,58 @@ init_subtree_with_zero (struct access *a
>        if (insert_after)
>         gsi_insert_after (gsi, ds, GSI_NEW_STMT);
>        else
>         gsi_insert_before (gsi, ds, GSI_SAME_STMT);
>      }
>
>    for (child = access->first_child; child; child = child->next_sibling)
>      init_subtree_with_zero (child, gsi, insert_after, loc);
>  }
>

Missing comment.

> +static void
> +clobber_subtree (struct access *access, gimple_stmt_iterator *gsi,
> +               bool insert_after, location_t loc)
> +
> +{
> +  struct access *child;
> +
> +  if (access->grp_to_be_replaced)
> +    {
> +      tree rep = get_access_replacement (access);
> +      tree clobber = build_constructor (access->type, NULL);
> +      TREE_THIS_VOLATILE (clobber) = 1;
> +      gimple stmt = gimple_build_assign (rep, clobber);
> +
> +      if (insert_after)
> +       gsi_insert_after (gsi, stmt, GSI_NEW_STMT);
> +      else
> +       gsi_insert_before (gsi, stmt, GSI_SAME_STMT);
> +      update_stmt (stmt);
> +      gimple_set_location (stmt, loc);
> +    }
> +  else if (access->grp_to_be_debug_replaced)
> +    {

Why would we care to create clobbers for debug stmts?!  Are those
even valid?

Otherwise this looks good I think.

Richard.

> +      tree rep = get_access_replacement (access);
> +      tree clobber = build_constructor (access->type, NULL);
> +      TREE_THIS_VOLATILE (clobber) = 1;
> +      gimple ds = gimple_build_debug_bind (rep, clobber, gsi_stmt (*gsi));
> +
> +      if (insert_after)
> +       gsi_insert_after (gsi, ds, GSI_NEW_STMT);
> +      else
> +       gsi_insert_before (gsi, ds, GSI_SAME_STMT);
> +    }
> +
> +  for (child = access->first_child; child; child = child->next_sibling)
> +    clobber_subtree (child, gsi, insert_after, loc);
> +}
> +
>  /* Search for an access representative for the given expression EXPR and
>     return it or NULL if it cannot be found.  */
>
>  static struct access *
>  get_access_for_expr (tree expr)
>  {
>    HOST_WIDE_INT offset, size, max_size;
>    tree base;
>
>    /* FIXME: This should not be necessary but Ada produces V_C_Es with a
> type of
> @@ -3039,43 +3077,41 @@ enum assignment_mod_result { SRA_AM_NONE
>                              SRA_AM_REMOVED };  /* stmt eliminated */
>
>  /* Modify assignments with a CONSTRUCTOR on their RHS.  STMT contains a
> pointer
>     to the assignment and GSI is the statement iterator pointing at it.
> Returns
>     the same values as sra_modify_assign.  */
>
>  static enum assignment_mod_result
>  sra_modify_constructor_assign (gimple *stmt, gimple_stmt_iterator *gsi)
>  {
>    tree lhs = gimple_assign_lhs (*stmt);
> -  struct access *acc;
> -  location_t loc;
> -
> -  acc = get_access_for_expr (lhs);
> +  struct access *acc = get_access_for_expr (lhs);
>    if (!acc)
>      return SRA_AM_NONE;
> +  location_t loc = gimple_location (*stmt);
>
>    if (gimple_clobber_p (*stmt))
>      {
> -      /* Remove clobbers of fully scalarized variables, otherwise
> -        do nothing.  */
> +      /* Clobber the replacement variable.  */
> +      clobber_subtree (acc, gsi, !acc->grp_covered, loc);
> +      /* Remove clobbers of fully scalarized variables, they are dead.  */
>        if (acc->grp_covered)
>         {
>           unlink_stmt_vdef (*stmt);
>           gsi_remove (gsi, true);
>           release_defs (*stmt);
>           return SRA_AM_REMOVED;
>         }
>        else
> -       return SRA_AM_NONE;
> +       return SRA_AM_MODIFIED;
>      }
>
> -  loc = gimple_location (*stmt);
>    if (vec_safe_length (CONSTRUCTOR_ELTS (gimple_assign_rhs1 (*stmt))) > 0)
>      {
>        /* I have never seen this code path trigger but if it can happen the
>          following should handle it gracefully.  */
>        if (access_has_children_p (acc))
>         generate_subtree_copies (acc->first_child, lhs, acc->offset, 0, 0,
> gsi,
>                                  true, true, loc);
>        return SRA_AM_MODIFIED;
>      }
>
> Index: gcc/testsuite/g++.dg/tree-ssa/pr60517.C
> ===================================================================
> --- gcc/testsuite/g++.dg/tree-ssa/pr60517.C     (revision 0)
> +++ gcc/testsuite/g++.dg/tree-ssa/pr60517.C     (working copy)
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -Wall" } */
> +
> +struct B {
> +    double x[2];
> +};
> +struct A {
> +    B b;
> +    B getB() { return b; }
> +};
> +
> +double foo(A a) {
> +    double * x = &(a.getB().x[0]);
> +    return x[0]; /* { dg-warning "" } */
> +}
>

Re: SRA: don't drop clobbers

[Jakub Jelinek]

On Thu, Jul 10, 2014 at 04:54:53PM +0200, Richard Biener wrote:
> > +  else if (access->grp_to_be_debug_replaced)
> > +    {
> 
> Why would we care to create clobbers for debug stmts?!  Are those
> even valid?

It is not valid.  Though, the fields supposedly live nowhere after the
clobber, so perhaps one could use
GIMPLE_DEBUG_BIND_NOVALUE instead of the clobber in the
gimple_build_debug_bind call, and drop the previous two lines.
Not sure if it is desirable though, it might cause the debug info to say
something is unavailable even if it still lives in some register or memory
for a few extra instructions.

Alex, what do you think?

> > +      tree rep = get_access_replacement (access);
> > +      tree clobber = build_constructor (access->type, NULL);
> > +      TREE_THIS_VOLATILE (clobber) = 1;
> > +      gimple ds = gimple_build_debug_bind (rep, clobber, gsi_stmt (*gsi));
> > +
> > +      if (insert_after)
> > +       gsi_insert_after (gsi, ds, GSI_NEW_STMT);
> > +      else
> > +       gsi_insert_before (gsi, ds, GSI_SAME_STMT);
> > +    }
> > +
> > +  for (child = access->first_child; child; child = child->next_sibling)
> > +    clobber_subtree (child, gsi, insert_after, loc);
> > +}
> > +

	Jakub

Re: update address taken: don't drop clobbers

[Jeff Law]

On 06/28/14 16:33, Marc Glisse wrote:
> In an earlier version of the patch, I was using
> get_or_create_ssa_default_def (cfun, sym);
> (I was reusing the same variable). This passed bootstrap+testsuite on
> all languages except for ada. Indeed, the compiler wanted to coalesce
> several SSA_NAMEs, including those new ones, in out-of-ssa, but
> couldn't.
And that's what you need to delve deeper into.  Why did it refuse to 
coalesce?

As long as the lifetimes do not overlap, then coalescing should have worked.

jeff

Re: update address taken: don't drop clobbers

[Marc Glisse]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2493 bytes --]

On Mon, 30 Jun 2014, Jeff Law wrote:

> On 06/28/14 16:33, Marc Glisse wrote:
>> In an earlier version of the patch, I was using
>> get_or_create_ssa_default_def (cfun, sym);
>> (I was reusing the same variable). This passed bootstrap+testsuite on
>> all languages except for ada. Indeed, the compiler wanted to coalesce
>> several SSA_NAMEs, including those new ones, in out-of-ssa, but
>> couldn't.
> And that's what you need to delve deeper into.  Why did it refuse to 
> coalesce?
>
> As long as the lifetimes do not overlap, then coalescing should have worked.

What is the lifetime of an SSA_NAME with a default definition? The way we 
handle it now, we start from the uses and go back to all blocks that can 
reach one of the uses, since there is no defining statement where we could 
stop (intuitively we should stop where the clobber was, if not earlier). 
This huge lifetime makes it very likely for such an SSA_NAME to conflict 
with others. And if an abnormal phi is involved, and thus we must 
coalesce, there is a problem.

The patch attached (it should probably use ssa_undefined_value_p with a 
new extra argument to say whether we care about partially-undefined) makes 
the lifetime of undefined local variables empty and lets the original 
patch work with:
       def = get_or_create_ssa_default_def (cfun, sym);
instead of creating a new variable.

However, I am not convinced reusing the same variable is necessarily the 
best thing. For warnings, we can create a new variable with the same name 
(with .1 added by gcc at the end) and copy the location info (I am not 
doing that yet), so little is lost. A new variable expresses more clearly 
that the value it holds is random crap. If I reuse the same variable, the 
SRA patch doesn't warn because SRA explicitly sets TREE_NO_WARNING (this 
can probably be changed, but that's starting to be a lot of changes). 
Creating a new variable is also more general. When reading *ssa_name after 
*ssa_name={v}{CLOBBER}; or after free(ssa_name); we have no variable to 
reuse so we will need to create a new undefined variable, and if a 
variable is global or a PARM_DECL, its default definition is not an 
undefined value (though it will probably happen in a different pass, so it 
doesn't have to behave the same).

(Side note: we don't seem to have any code to notice that:
a=phi<undef,b>
b=phi<undef,a>
means both phis can be replaced with undefined variables.)

Do you have any advice on the right direction?

-- 
Marc Glisse

[-- Attachment #2: Type: TEXT/PLAIN, Size: 1978 bytes --]

Index: gcc/tree-ssa-live.c
===================================================================
--- gcc/tree-ssa-live.c	(revision 212306)
+++ gcc/tree-ssa-live.c	(working copy)
@@ -1086,20 +1086,28 @@ set_var_live_on_entry (tree ssa_name, tr
   if (stmt)
     {
       def_bb = gimple_bb (stmt);
       /* Mark defs in liveout bitmap temporarily.  */
       if (def_bb)
 	bitmap_set_bit (&live->liveout[def_bb->index], p);
     }
   else
     def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
 
+  /* An undefined local variable does not need to be very alive.  */
+  if (SSA_NAME_IS_DEFAULT_DEF (ssa_name))
+    {
+      tree var = SSA_NAME_VAR (ssa_name);
+      if (var && TREE_CODE (var) == VAR_DECL && !is_global_var (var))
+	return;
+    }
+
   /* Visit each use of SSA_NAME and if it isn't in the same block as the def,
      add it to the list of live on entry blocks.  */
   FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
     {
       gimple use_stmt = USE_STMT (use);
       basic_block add_block = NULL;
 
       if (gimple_code (use_stmt) == GIMPLE_PHI)
         {
 	  /* Uses in PHI's are considered to be live at exit of the SRC block
@@ -1422,20 +1430,27 @@ verify_live_on_entry (tree_live_info_p l
 			  fprintf (stderr, "\n");
 			}
 		      else
 			fprintf (stderr, " and there is no default def.\n");
 		    }
 		}
 	    }
 	  else
 	    if (d == var)
 	      {
+		/* An undefined local variable does not need to be very
+		   alive.  */
+		tree real_var = SSA_NAME_VAR (var);
+		if (real_var && TREE_CODE (real_var) == VAR_DECL
+		    && !is_global_var (real_var))
+		  continue;
+
 		/* The only way this var shouldn't be marked live on entry is
 		   if it occurs in a PHI argument of the block.  */
 		size_t z;
 		bool ok = false;
 		gimple_stmt_iterator gsi;
 		for (gsi = gsi_start_phis (e->dest);
 		     !gsi_end_p (gsi) && !ok;
 		     gsi_next (&gsi))
 		  {
 		    gimple phi = gsi_stmt (gsi);

Re: update address taken: don't drop clobbers

[pinskia]

> On Jul 6, 2014, at 7:23 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> 
>> On Mon, 30 Jun 2014, Jeff Law wrote:
>> 
>>> On 06/28/14 16:33, Marc Glisse wrote:
>>> In an earlier version of the patch, I was using
>>> get_or_create_ssa_default_def (cfun, sym);
>>> (I was reusing the same variable). This passed bootstrap+testsuite on
>>> all languages except for ada. Indeed, the compiler wanted to coalesce
>>> several SSA_NAMEs, including those new ones, in out-of-ssa, but
>>> couldn't.
>> And that's what you need to delve deeper into.  Why did it refuse to coalesce?
>> 
>> As long as the lifetimes do not overlap, then coalescing should have worked.
> 
> What is the lifetime of an SSA_NAME with a default definition? The way we handle it now, we start from the uses and go back to all blocks that can reach one of the uses, since there is no defining statement where we could stop (intuitively we should stop where the clobber was, if not earlier). This huge lifetime makes it very likely for such an SSA_NAME to conflict with others. And if an abnormal phi is involved, and thus we must coalesce, there is a problem.
> 
> The patch attached (it should probably use ssa_undefined_value_p with a new extra argument to say whether we care about partially-undefined) makes the lifetime of undefined local variables empty and lets the original patch work with:
>      def = get_or_create_ssa_default_def (cfun, sym);
> instead of creating a new variable.
> 
> However, I am not convinced reusing the same variable is necessarily the best thing. For warnings, we can create a new variable with the same name (with .1 added by gcc at the end) and copy the location info (I am not doing that yet), so little is lost. A new variable expresses more clearly that the value it holds is random crap. If I reuse the same variable, the SRA patch doesn't warn because SRA explicitly sets TREE_NO_WARNING (this can probably be changed, but that's starting to be a lot of changes). Creating a new variable is also more general. When reading *ssa_name after *ssa_name={v}{CLOBBER}; or after free(ssa_name); we have no variable to reuse so we will need to create a new undefined variable, and if a variable is global or a PARM_DECL, its default definition is not an undefined value (though it will probably happen in a different pass, so it doesn't have to behave the same).
> 
> (Side note: we don't seem to have any code to notice that:
> a=phi<undef,b>
> b=phi<undef,a>
> means both phis can be replaced with undefined variables.)
> 
> Do you have any advice on the right direction?

The below patch won't work for parameters. 

Thanks,
Andrew

> 
> -- 
> Marc Glisse
> Index: gcc/tree-ssa-live.c
> ===================================================================
> --- gcc/tree-ssa-live.c    (revision 212306)
> +++ gcc/tree-ssa-live.c    (working copy)
> @@ -1086,20 +1086,28 @@ set_var_live_on_entry (tree ssa_name, tr
>   if (stmt)
>     {
>       def_bb = gimple_bb (stmt);
>       /* Mark defs in liveout bitmap temporarily.  */
>       if (def_bb)
>    bitmap_set_bit (&live->liveout[def_bb->index], p);
>     }
>   else
>     def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
> 
> +  /* An undefined local variable does not need to be very alive.  */
> +  if (SSA_NAME_IS_DEFAULT_DEF (ssa_name))
> +    {
> +      tree var = SSA_NAME_VAR (ssa_name);
> +      if (var && TREE_CODE (var) == VAR_DECL && !is_global_var (var))
> +    return;
> +    }
> +
>   /* Visit each use of SSA_NAME and if it isn't in the same block as the def,
>      add it to the list of live on entry blocks.  */
>   FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
>     {
>       gimple use_stmt = USE_STMT (use);
>       basic_block add_block = NULL;
> 
>       if (gimple_code (use_stmt) == GIMPLE_PHI)
>         {
>      /* Uses in PHI's are considered to be live at exit of the SRC block
> @@ -1422,20 +1430,27 @@ verify_live_on_entry (tree_live_info_p l
>              fprintf (stderr, "\n");
>            }
>              else
>            fprintf (stderr, " and there is no default def.\n");
>            }
>        }
>        }
>      else
>        if (d == var)
>          {
> +        /* An undefined local variable does not need to be very
> +           alive.  */
> +        tree real_var = SSA_NAME_VAR (var);
> +        if (real_var && TREE_CODE (real_var) == VAR_DECL
> +            && !is_global_var (real_var))
> +          continue;
> +
>        /* The only way this var shouldn't be marked live on entry is
>           if it occurs in a PHI argument of the block.  */
>        size_t z;
>        bool ok = false;
>        gimple_stmt_iterator gsi;
>        for (gsi = gsi_start_phis (e->dest);
>             !gsi_end_p (gsi) && !ok;
>             gsi_next (&gsi))
>          {
>            gimple phi = gsi_stmt (gsi);

Re: update address taken: don't drop clobbers

[Marc Glisse]

On Sun, 6 Jul 2014, pinskia@gmail.com wrote:

> The below patch won't work for parameters.

Er, that's why I am testing: TREE_CODE (var) == VAR_DECL
(and the patch passed the testsuite with all languages)

Could you be more specific about what won't work?

-- 
Marc Glisse

Re: update address taken: don't drop clobbers

[Richard Biener]

On Sun, Jul 6, 2014 at 4:23 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Mon, 30 Jun 2014, Jeff Law wrote:
>
>> On 06/28/14 16:33, Marc Glisse wrote:
>>>
>>> In an earlier version of the patch, I was using
>>> get_or_create_ssa_default_def (cfun, sym);
>>> (I was reusing the same variable). This passed bootstrap+testsuite on
>>> all languages except for ada. Indeed, the compiler wanted to coalesce
>>> several SSA_NAMEs, including those new ones, in out-of-ssa, but
>>> couldn't.
>>
>> And that's what you need to delve deeper into.  Why did it refuse to
>> coalesce?
>>
>> As long as the lifetimes do not overlap, then coalescing should have
>> worked.
>
>
> What is the lifetime of an SSA_NAME with a default definition? The way we
> handle it now, we start from the uses and go back to all blocks that can
> reach one of the uses, since there is no defining statement where we could
> stop (intuitively we should stop where the clobber was, if not earlier).
> This huge lifetime makes it very likely for such an SSA_NAME to conflict
> with others. And if an abnormal phi is involved, and thus we must coalesce,
> there is a problem.
>
> The patch attached (it should probably use ssa_undefined_value_p with a new
> extra argument to say whether we care about partially-undefined) makes the
> lifetime of undefined local variables empty and lets the original patch work
> with:
>       def = get_or_create_ssa_default_def (cfun, sym);
> instead of creating a new variable.
>
> However, I am not convinced reusing the same variable is necessarily the
> best thing. For warnings, we can create a new variable with the same name
> (with .1 added by gcc at the end) and copy the location info (I am not doing
> that yet), so little is lost. A new variable expresses more clearly that the
> value it holds is random crap. If I reuse the same variable, the SRA patch
> doesn't warn because SRA explicitly sets TREE_NO_WARNING (this can probably
> be changed, but that's starting to be a lot of changes). Creating a new
> variable is also more general. When reading *ssa_name after
> *ssa_name={v}{CLOBBER}; or after free(ssa_name); we have no variable to
> reuse so we will need to create a new undefined variable, and if a variable
> is global or a PARM_DECL, its default definition is not an undefined value
> (though it will probably happen in a different pass, so it doesn't have to
> behave the same).
>
> (Side note: we don't seem to have any code to notice that:
> a=phi<undef,b>
> b=phi<undef,a>
> means both phis can be replaced with undefined variables.)

Propagators will never replace sth with unefined but they will exploit
the undefinedness.  Constant propagation is even a bit more aggressive.

> Do you have any advice on the right direction?

The patch below would work (btw, please use ssa_undefined_value_p),
but I wonder if it's the conflict code that should get adjustments
rather than the lifeness computation (lifeness of undefined values
shouldn't be needed at all).

Also depends on how we expand an undefined value of course.

Richard.

> --
> Marc Glisse
> Index: gcc/tree-ssa-live.c
> ===================================================================
> --- gcc/tree-ssa-live.c (revision 212306)
> +++ gcc/tree-ssa-live.c (working copy)
> @@ -1086,20 +1086,28 @@ set_var_live_on_entry (tree ssa_name, tr
>    if (stmt)
>      {
>        def_bb = gimple_bb (stmt);
>        /* Mark defs in liveout bitmap temporarily.  */
>        if (def_bb)
>         bitmap_set_bit (&live->liveout[def_bb->index], p);
>      }
>    else
>      def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
>
> +  /* An undefined local variable does not need to be very alive.  */
> +  if (SSA_NAME_IS_DEFAULT_DEF (ssa_name))
> +    {
> +      tree var = SSA_NAME_VAR (ssa_name);
> +      if (var && TREE_CODE (var) == VAR_DECL && !is_global_var (var))
> +       return;
> +    }
> +
>    /* Visit each use of SSA_NAME and if it isn't in the same block as the
> def,
>       add it to the list of live on entry blocks.  */
>    FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
>      {
>        gimple use_stmt = USE_STMT (use);
>        basic_block add_block = NULL;
>
>        if (gimple_code (use_stmt) == GIMPLE_PHI)
>          {
>           /* Uses in PHI's are considered to be live at exit of the SRC
> block
> @@ -1422,20 +1430,27 @@ verify_live_on_entry (tree_live_info_p l
>                           fprintf (stderr, "\n");
>                         }
>                       else
>                         fprintf (stderr, " and there is no default def.\n");
>                     }
>                 }
>             }
>           else
>             if (d == var)
>               {
> +               /* An undefined local variable does not need to be very
> +                  alive.  */
> +               tree real_var = SSA_NAME_VAR (var);
> +               if (real_var && TREE_CODE (real_var) == VAR_DECL
> +                   && !is_global_var (real_var))
> +                 continue;
> +
>                 /* The only way this var shouldn't be marked live on entry
> is
>                    if it occurs in a PHI argument of the block.  */
>                 size_t z;
>                 bool ok = false;
>                 gimple_stmt_iterator gsi;
>                 for (gsi = gsi_start_phis (e->dest);
>                      !gsi_end_p (gsi) && !ok;
>                      gsi_next (&gsi))
>                   {
>                     gimple phi = gsi_stmt (gsi);
>

Re: update address taken: don't drop clobbers

[Jeff Law]

On 07/06/14 08:23, Marc Glisse wrote:
> What is the lifetime of an SSA_NAME with a default definition? The way
> we handle it now, we start from the uses and go back to all blocks that
> can reach one of the uses, since there is no defining statement where we
> could stop
Right, that's exactly what I would expect given the typical definition 
of liveness.


  (intuitively we should stop where the clobber was, if not
> earlier). This huge lifetime makes it very likely for such an SSA_NAME
> to conflict with others. And if an abnormal phi is involved, and thus we
> must coalesce, there is a problem.
So this suggests another approach.  Could we just assume that it's 
always safe to coalesce with a default definition?


>
> The patch attached (it should probably use ssa_undefined_value_p with a
> new extra argument to say whether we care about partially-undefined)
> makes the lifetime of undefined local variables empty and lets the
> original patch work with:
>        def = get_or_create_ssa_default_def (cfun, sym);
> instead of creating a new variable.
Not a terrible suggestion either and accomplishes the same thing as the 
mental model I had of special casing this stuff in the coalescing code.

I guess this is the first thing we probably should sort out.  Do we want 
to handle this is the conflict, coalescing or lifetime analysis.

We've certainly had similar hacks in the code which built the conflict 
matrix for the register allocator in the past (old local-alloc/global 
variant, pre IRA).   By not recoding those conflicts, coalescing just 
magically works.

By handling it when we build the lifetimes, they'll magically coalesce 
because there's no conflicts as well.  It may help other code which 
utilizes lifetimes as well.  But a part of me doesn't like it because it 
is different than the traditionally formulated life analysis.

The question I havent thought much about is would the coalescing code do 
the right thing if we have one of these undefined SSA_NAMEs that 
coalesce into two different partitions.

ie, let's say we have two PHI nodes in different blocks.  Each PHI has 
one or more source/dest operations that are marked as 
SSA_NAME_OCCURS_IN_ABNORMAL_PHI.  Furthermore, they all reference a 
single RHS undefined SSA_NAME.

When we coalesce the undefined SSA_NAME with all the others in the first 
PHI, doesn't that mean that we then have to coalesce all the SSA_NAMES 
in both PHIs together (because that undefined SSA_NAME appears on the 
RHS on the 2nd PHI too).

Does this then argue that each use of an undefined SSA_NAME should be a 
unique SSA_NAME?

Ugh, I wonder if I just opened a can of worms here...

>
> However, I am not convinced reusing the same variable is necessarily the
> best thing. For warnings, we can create a new variable with the same
> name (with .1 added by gcc at the end) and copy the location info (I am
> not doing that yet), so little is lost. A new variable expresses more
> clearly that the value it holds is random crap. If I reuse the same
> variable, the SRA patch doesn't warn because SRA explicitly sets
> TREE_NO_WARNING (this can probably be changed, but that's starting to be
> a lot of changes). Creating a new variable is also more general. When
> reading *ssa_name after *ssa_name={v}{CLOBBER}; or after free(ssa_name);
> we have no variable to reuse so we will need to create a new undefined
> variable, and if a variable is global or a PARM_DECL, its default
> definition is not an undefined value (though it will probably happen in
> a different pass, so it doesn't have to behave the same).
My concern about using another variable was primarily that it was being 
used to hide a deeper issue.


>
> (Side note: we don't seem to have any code to notice that:
> a=phi<undef,b>
> b=phi<undef,a>
> means both phis can be replaced with undefined variables.)
Various passes will consider the undef value to be whatever value is 
convenient for optimization.  So for the first, we'd consider the undef 
value to be the same as "b" because that allows the PHI to be propagated 
away.

Obviously when there are many different values in the RHS, it's a lot 
less likely that we'll be able to propagate away the PHI.

Jeff

Re: update address taken: don't drop clobbers

[Marc Glisse]

On Mon, 7 Jul 2014, Jeff Law wrote:

> On 07/06/14 08:23, Marc Glisse wrote:
>> What is the lifetime of an SSA_NAME with a default definition? The way
>> we handle it now, we start from the uses and go back to all blocks that
>> can reach one of the uses, since there is no defining statement where we
>> could stop
> Right, that's exactly what I would expect given the typical definition of 
> liveness.
>
>
>> (intuitively we should stop where the clobber was, if not
>> earlier). This huge lifetime makes it very likely for such an SSA_NAME
>> to conflict with others. And if an abnormal phi is involved, and thus we
>> must coalesce, there is a problem.
> So this suggests another approach.  Could we just assume that it's always 
> safe to coalesce with a default definition?
>
>
>> The patch attached (it should probably use ssa_undefined_value_p with a
>> new extra argument to say whether we care about partially-undefined)
>> makes the lifetime of undefined local variables empty and lets the
>> original patch work with:
>>        def = get_or_create_ssa_default_def (cfun, sym);
>> instead of creating a new variable.
> Not a terrible suggestion either and accomplishes the same thing as the 
> mental model I had of special casing this stuff in the coalescing code.
>
> I guess this is the first thing we probably should sort out.  Do we want to 
> handle this is the conflict, coalescing or lifetime analysis.
>
> We've certainly had similar hacks in the code which built the conflict matrix 
> for the register allocator in the past (old local-alloc/global variant, pre 
> IRA).   By not recoding those conflicts, coalescing just magically works.
>
> By handling it when we build the lifetimes, they'll magically coalesce 
> because there's no conflicts as well.  It may help other code which utilizes 
> lifetimes as well.  But a part of me doesn't like it because it is different 
> than the traditionally formulated life analysis.

Those variables don't have a birth time so their lifetime is quite
ill-defined in any case. I first tried to force coalesce for those
variables, but it is not very convenient. We do want to consider them
for coalescing, we can't just ignore them. But when we coalesce them
with a first variable, we want to mark it somehow so we don't later
coalesce them with a second variable that conflicts with the first. The
way we do that normally is by computing the union of the lifetimes, but
if we ignore the lifetime for this variable... It isn't just a matter of
adding || ssa_undefined_value_p (var) in the conflict test. Giving them
an empty lifetime to begin with seems to do the right thing. If we don't
modify the lifetime computation, we would have in coalesce to pass only
the "defined" variables to calculate_live_ranges and re-add the
undefined variables afterwards with an empty range.

> The question I havent thought much about is would the coalescing code
> do the right thing if we have one of these undefined SSA_NAMEs that
> coalesce into two different partitions.
>
> ie, let's say we have two PHI nodes in different blocks.  Each PHI has
> one or more source/dest operations that are marked as
> SSA_NAME_OCCURS_IN_ABNORMAL_PHI.  Furthermore, they all reference a
> single RHS undefined SSA_NAME.
>
> When we coalesce the undefined SSA_NAME with all the others in the
> first PHI, doesn't that mean that we then have to coalesce all the
> SSA_NAMES in both PHIs together (because that undefined SSA_NAME
> appears on the RHS on the 2nd PHI too).
>
> Does this then argue that each use of an undefined SSA_NAME should be
> a unique SSA_NAME?

I did wonder about the same thing. But without the patch, there were only 
2 files in the whole gcc+testsuite that had an issue (in the ada 
front-end), and with the patch they were fine, so if the situation you 
describe is possible, at least it is very uncommon.

Creating a new variable instead of reusing the old one may be less
likely to cause this kind of trouble. At least at creation time we know
there is no conflict since it is coming from a variable by an operation
that is the reverse of coalescing.

> Ugh, I wonder if I just opened a can of worms here...
>
>> 
>> However, I am not convinced reusing the same variable is necessarily the
>> best thing. For warnings, we can create a new variable with the same
>> name (with .1 added by gcc at the end) and copy the location info (I am
>> not doing that yet), so little is lost. A new variable expresses more
>> clearly that the value it holds is random crap. If I reuse the same
>> variable, the SRA patch doesn't warn because SRA explicitly sets
>> TREE_NO_WARNING (this can probably be changed, but that's starting to be
>> a lot of changes). Creating a new variable is also more general. When
>> reading *ssa_name after *ssa_name={v}{CLOBBER}; or after free(ssa_name);
>> we have no variable to reuse so we will need to create a new undefined
>> variable, and if a variable is global or a PARM_DECL, its default
>> definition is not an undefined value (though it will probably happen in
>> a different pass, so it doesn't have to behave the same).
> My concern about using another variable was primarily that it was being used 
> to hide a deeper issue.
>
>
>> 
>> (Side note: we don't seem to have any code to notice that:
>> a=phi<undef,b>
>> b=phi<undef,a>
>> means both phis can be replaced with undefined variables.)
> Various passes will consider the undef value to be whatever value is 
> convenient for optimization.  So for the first, we'd consider the undef value 
> to be the same as "b" because that allows the PHI to be propagated away.

Considering that a is the same as b and b is the same as a is not as good 
as knowing that a and b are both undefined. I don't know if CCP is clever 
enough to still optimize if b is used in a further PHI, but it would have 
an easier time (and coalesce and the uninit warning as well) if a=undef; 
b=undef; were forwarded into their uses instead. Not a priority though.

> Obviously when there are many different values in the RHS, it's a lot less 
> likely that we'll be able to propagate away the PHI.


On Mon, 7 Jul 2014, Richard Biener wrote:

> On Sun, Jul 6, 2014 at 4:23 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> On Mon, 30 Jun 2014, Jeff Law wrote:
>>
>>> On 06/28/14 16:33, Marc Glisse wrote:
>>>>
>>>> In an earlier version of the patch, I was using
>>>> get_or_create_ssa_default_def (cfun, sym);
>>>> (I was reusing the same variable). This passed bootstrap+testsuite on
>>>> all languages except for ada. Indeed, the compiler wanted to coalesce
>>>> several SSA_NAMEs, including those new ones, in out-of-ssa, but
>>>> couldn't.
>>>
>>> And that's what you need to delve deeper into.  Why did it refuse to
>>> coalesce?
>>>
>>> As long as the lifetimes do not overlap, then coalescing should have
>>> worked.
>>
>>
>> What is the lifetime of an SSA_NAME with a default definition? The way we
>> handle it now, we start from the uses and go back to all blocks that can
>> reach one of the uses, since there is no defining statement where we could
>> stop (intuitively we should stop where the clobber was, if not earlier).
>> This huge lifetime makes it very likely for such an SSA_NAME to conflict
>> with others. And if an abnormal phi is involved, and thus we must coalesce,
>> there is a problem.
>>
>> The patch attached (it should probably use ssa_undefined_value_p with a new
>> extra argument to say whether we care about partially-undefined) makes the
>> lifetime of undefined local variables empty and lets the original patch work
>> with:
>>       def = get_or_create_ssa_default_def (cfun, sym);
>> instead of creating a new variable.
>>
>> However, I am not convinced reusing the same variable is necessarily the
>> best thing. For warnings, we can create a new variable with the same name
>> (with .1 added by gcc at the end) and copy the location info (I am not doing
>> that yet), so little is lost. A new variable expresses more clearly that the
>> value it holds is random crap. If I reuse the same variable, the SRA patch
>> doesn't warn because SRA explicitly sets TREE_NO_WARNING (this can probably
>> be changed, but that's starting to be a lot of changes). Creating a new
>> variable is also more general. When reading *ssa_name after
>> *ssa_name={v}{CLOBBER}; or after free(ssa_name); we have no variable to
>> reuse so we will need to create a new undefined variable, and if a variable
>> is global or a PARM_DECL, its default definition is not an undefined value
>> (though it will probably happen in a different pass, so it doesn't have to
>> behave the same).
>>
>> (Side note: we don't seem to have any code to notice that:
>> a=phi<undef,b>
>> b=phi<undef,a>
>> means both phis can be replaced with undefined variables.)
>
> Propagators will never replace sth with unefined but they will exploit
> the undefinedness.  Constant propagation is even a bit more aggressive.
>
>> Do you have any advice on the right direction?
>
> The patch below would work (btw, please use ssa_undefined_value_p),
> but I wonder if it's the conflict code that should get adjustments
> rather than the lifeness computation (lifeness of undefined values
> shouldn't be needed at all).
>
> Also depends on how we expand an undefined value of course.

I didn't really look at the expansion part.

-- 
Marc Glisse

Re: update address taken: don't drop clobbers

[Richard Biener]

On Sun, Jun 29, 2014 at 12:33 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Hello,
>
> we currently drop clobbers on variables whose address is not taken anymore.
> However, rewrite_stmt has code to replace them with an SSA_NAME with a
> default definition (an uninitialized variable), and I believe
> rewrite_update_stmt should do the same. This allows us to warn sometimes
> (see testcase), but during the debugging I also noticed several places where
> it allowed CCP to simplify further PHIs, so this is also an optimization.
>
> In an earlier version of the patch, I was using
> get_or_create_ssa_default_def (cfun, sym);
> (I was reusing the same variable). This passed bootstrap+testsuite on all
> languages except for ada. Indeed, the compiler wanted to coalesce several
> SSA_NAMEs, including those new ones, in out-of-ssa, but couldn't. There are
> abnormal PHIs involved. Maybe it shouldn't have insisted on coalescing an
> undefined ssa_name, maybe something should have prevented us from reaching
> such a situation, but creating a new variable was the simplest workaround.

Hmm.  We indeed notice "late" that the new names are used in abnormal
PHIs.  Note that usually rewriting a symbol into SSA form does not
create overlapping life-ranges - but of course you are possibly introducing
those by the new use of the default definitions.

Apart from the out-of-SSA patch you proposed elsewhere a possibility
is to simply never mark undefined SSA names as
SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
as must-coalesce).

> Some things could be done to improve the error message in uninit:
> - getting the location of the variable,
> - differenciating uninitialized from clobbered,
> but that can come later.
>
> Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-unknown-linux-gnu.
>
> 2014-06-30  Marc Glisse  <marc.glisse@inria.fr>
>
>         PR tree-optimization/60770
> gcc/
>         * tree-ssa.c (execute_update_addresses_taken): Don't drop clobbers.
>         * tree-into-ssa.c (maybe_register_def): Replace clobbers with a
>         default definition.
> gcc/testsuite/
>         * gcc.dg/tree-ssa/pr60770-1.c: New file.
>
> --
> Marc Glisse
> Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c   (revision 0)
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c   (working copy)
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -Wall" } */
> +
> +int f(int n){
> +  int*p;
> +  {
> +    int yyy=n;
> +    p=&yyy;
> +  }
> +  return *p; /* { dg-warning "yyy" } */
> +}
> Index: gcc/tree-into-ssa.c
> ===================================================================
> --- gcc/tree-into-ssa.c (revision 212109)
> +++ gcc/tree-into-ssa.c (working copy)
> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
>  {
>    tree def = DEF_FROM_PTR (def_p);
>    tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>
>    /* If DEF is a naked symbol that needs renaming, create a new
>       name for it.  */
>    if (marked_for_renaming (sym))
>      {
>        if (DECL_P (def))
>         {
> -         tree tracked_var;
> -
> -         def = make_ssa_name (def, stmt);
> +         if (gimple_clobber_p (stmt) && is_gimple_reg (sym))

sym should always be a gimple reg here (it's marked for renaming).

> +           {
> +             /* Replace clobber stmts with a default def.  Create a new
> +                variable so we don't later think we must coalesce, which
> would
> +                fail with some ada abnormal PHIs.  Still, we try to keep a
> +                similar name so error messages make sense.  */
> +             unlink_stmt_vdef (stmt);

I think that's redundant with gsi_replace (note that using gsi_replace
looks dangerous here as it calls update_stmt during SSA rewrite...
that might open a can of worms).

> +             gsi_replace (&gsi, gimple_build_nop (), true);
> +             tree id = DECL_NAME (sym);
> +             const char* name = id ? IDENTIFIER_POINTER (id) : 0;
> +             tree newvar = create_tmp_var (TREE_TYPE (sym), name);
> +             def = get_or_create_ssa_default_def (cfun, newvar);

So - can't you simply do

    gimple_assign_set_rhs_from_tree (&gsi,
get_or_create_dda_default_def (cfun, sym));

?  Thus replace x = CLOBBER; with x_3 = x_2(D);

> +           }
> +         else

and of course still rewrite the DEF then.  IMHO the copy-propagation
you do is premature optimization.

> +           def = make_ssa_name (def, stmt);
>           SET_DEF (def_p, def);
>
> -         tracked_var = target_for_debug_bind (sym);
> +         tree tracked_var = target_for_debug_bind (sym);
>           if (tracked_var)
>             {
>               gimple note = gimple_build_debug_bind (tracked_var, def,
> stmt);
>               /* If stmt ends the bb, insert the debug stmt on the single
>                  non-EH edge from the stmt.  */
>               if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
>                 {
>                   basic_block bb = gsi_bb (gsi);
>                   edge_iterator ei;
>                   edge e, ef = NULL;
> Index: gcc/tree-ssa.c
> ===================================================================
> --- gcc/tree-ssa.c      (revision 212109)
> +++ gcc/tree-ssa.c      (working copy)
> @@ -1607,32 +1607,20 @@ execute_update_addresses_taken (void)
>                 rhs = gimple_assign_rhs1 (stmt);
>                 if (gimple_assign_lhs (stmt) != lhs
>                     && !useless_type_conversion_p (TREE_TYPE (lhs),
>                                                    TREE_TYPE (rhs)))
>                   rhs = fold_build1 (VIEW_CONVERT_EXPR,
>                                      TREE_TYPE (lhs), rhs);
>
>                 if (gimple_assign_lhs (stmt) != lhs)
>                   gimple_assign_set_lhs (stmt, lhs);
>
> -               /* For var ={v} {CLOBBER}; where var lost
> -                  TREE_ADDRESSABLE just remove the stmt.  */
> -               if (DECL_P (lhs)
> -                   && TREE_CLOBBER_P (rhs)
> -                   && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
> -                 {
> -                   unlink_stmt_vdef (stmt);
> -                   gsi_remove (&gsi, true);
> -                   release_defs (stmt);
> -                   continue;
> -                 }
> -
>                 if (gimple_assign_rhs1 (stmt) != rhs)
>                   {
>                     gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
>                     gimple_assign_set_rhs_from_tree (&gsi, rhs);
>                   }
>               }
>
>             else if (gimple_code (stmt) == GIMPLE_CALL)
>               {
>                 unsigned i;
>

Re: update address taken: don't drop clobbers

[Michael Matz]

Hi,

On Thu, 10 Jul 2014, Richard Biener wrote:

> Apart from the out-of-SSA patch you proposed elsewhere a possibility
> is to simply never mark undefined SSA names as
> SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
> as must-coalesce).

The insight to note is, that undefined SSA names should really be 
coalesced with something (otherwise you lost an optimization opportunity), 
but it doesn't matter with _what_ each use of the undefined name is 
coalesced, you can even identify different uses of them with different SSA 
names (e.g. the LHS of each using stmt).  Requires some change in the 
order things are done in out-of-ssa.


Ciao,
Michael.

Re: update address taken: don't drop clobbers

[Jeff Law]

On 07/10/14 09:48, Michael Matz wrote:
> Hi,
>
> On Thu, 10 Jul 2014, Richard Biener wrote:
>
>> Apart from the out-of-SSA patch you proposed elsewhere a possibility
>> is to simply never mark undefined SSA names as
>> SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
>> as must-coalesce).
>
> The insight to note is, that undefined SSA names should really be
> coalesced with something (otherwise you lost an optimization opportunity),
> but it doesn't matter with _what_ each use of the undefined name is
> coalesced, you can even identify different uses of them with different SSA
> names (e.g. the LHS of each using stmt).  Requires some change in the
> order things are done in out-of-ssa.
The last part is what I hinted might be problematical.  If some 
undefined SSA_NAME appears on the RHS of two PHIs and we want to 
coalesce that undefined SSA_NAME with the LHS of each of those PHIs, 
then the LHS of those two PHIs must coalesce as well.  At least that's 
my recollection of how all that stuff worked.

It was that realization that made me wonder if we should have a unique 
SSA_NAME at each undefined use point.

jeff

Re: update address taken: don't drop clobbers

[Richard Biener]

On Thu, Jul 10, 2014 at 8:22 PM, Jeff Law <law@redhat.com> wrote:
> On 07/10/14 09:48, Michael Matz wrote:
>>
>> Hi,
>>
>> On Thu, 10 Jul 2014, Richard Biener wrote:
>>
>>> Apart from the out-of-SSA patch you proposed elsewhere a possibility
>>> is to simply never mark undefined SSA names as
>>> SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
>>> as must-coalesce).
>>
>>
>> The insight to note is, that undefined SSA names should really be
>> coalesced with something (otherwise you lost an optimization opportunity),
>> but it doesn't matter with _what_ each use of the undefined name is
>> coalesced, you can even identify different uses of them with different SSA
>> names (e.g. the LHS of each using stmt).  Requires some change in the
>> order things are done in out-of-ssa.
>
> The last part is what I hinted might be problematical.  If some undefined
> SSA_NAME appears on the RHS of two PHIs and we want to coalesce that
> undefined SSA_NAME with the LHS of each of those PHIs, then the LHS of those
> two PHIs must coalesce as well.  At least that's my recollection of how all
> that stuff worked.

Yes, coalescing doesn't do "live-range splitting" to avoid coalescing the
two PHI results.  But they have to be coalesced anyway.

I still think simply never recording conflicts for undefined SSA names
is a proper "hack" to avoid this issue.

> It was that realization that made me wonder if we should have a unique
> SSA_NAME at each undefined use point.

That would be unnecessarily expensive.

Richard.

> jeff
>

Re: update address taken: don't drop clobbers

[Richard Biener]

On Fri, Jul 11, 2014 at 10:10 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Jul 10, 2014 at 8:22 PM, Jeff Law <law@redhat.com> wrote:
>> On 07/10/14 09:48, Michael Matz wrote:
>>>
>>> Hi,
>>>
>>> On Thu, 10 Jul 2014, Richard Biener wrote:
>>>
>>>> Apart from the out-of-SSA patch you proposed elsewhere a possibility
>>>> is to simply never mark undefined SSA names as
>>>> SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
>>>> as must-coalesce).
>>>
>>>
>>> The insight to note is, that undefined SSA names should really be
>>> coalesced with something (otherwise you lost an optimization opportunity),
>>> but it doesn't matter with _what_ each use of the undefined name is
>>> coalesced, you can even identify different uses of them with different SSA
>>> names (e.g. the LHS of each using stmt).  Requires some change in the
>>> order things are done in out-of-ssa.
>>
>> The last part is what I hinted might be problematical.  If some undefined
>> SSA_NAME appears on the RHS of two PHIs and we want to coalesce that
>> undefined SSA_NAME with the LHS of each of those PHIs, then the LHS of those
>> two PHIs must coalesce as well.  At least that's my recollection of how all
>> that stuff worked.
>
> Yes, coalescing doesn't do "live-range splitting" to avoid coalescing the
> two PHI results.  But they have to be coalesced anyway.
>
> I still think simply never recording conflicts for undefined SSA names
> is a proper "hack" to avoid this issue.
>
>> It was that realization that made me wonder if we should have a unique
>> SSA_NAME at each undefined use point.
>
> That would be unnecessarily expensive.

Btw, the bug must be already kind-of preexisting due to

bool
may_propagate_copy (tree dest, tree orig)
{
...
  /* If ORIG flows in from an abnormal edge, it cannot be propagated.  */
  if (TREE_CODE (orig) == SSA_NAME
      && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig)
      /* If it is the default definition and an automatic variable then
         we can though and it is important that we do to avoid
         uninitialized regular copies.  */
      && !(SSA_NAME_IS_DEFAULT_DEF (orig)
           && (SSA_NAME_VAR (orig) == NULL_TREE
               || TREE_CODE (SSA_NAME_VAR (orig)) == VAR_DECL)))
    return false;

but it never replaces an abnormal SSA names with its default definition
due to the next check:

  /* If DEST is an SSA_NAME that flows from an abnormal edge, then it
     cannot be replaced.  */
  if (TREE_CODE (dest) == SSA_NAME
      && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (dest))
    return false;

still it might create overlapping life-ranges for abnormals.

Richard.

> Richard.
>
>> jeff
>>

Re: update address taken: don't drop clobbers

[Michael Matz]

Hi,

On Thu, 10 Jul 2014, Jeff Law wrote:

> > The insight to note is, that undefined SSA names should really be
> > coalesced with something (otherwise you lost an optimization opportunity),
> > but it doesn't matter with _what_ each use of the undefined name is
> > coalesced, you can even identify different uses of them with different SSA
> > names (e.g. the LHS of each using stmt).  Requires some change in the
> > order things are done in out-of-ssa.
> 
> The last part is what I hinted might be problematical.  If some 
> undefined SSA_NAME appears on the RHS of two PHIs and we want to 
> coalesce that undefined SSA_NAME with the LHS of each of those PHIs, 
> then the LHS of those two PHIs must coalesce as well.  At least that's 
> my recollection of how all that stuff worked.

Only with the usual definition of coalescing (being transitive).  For 
undefined SSA names the definition can be mended.

> It was that realization that made me wonder if we should have a unique 
> SSA_NAME at each undefined use point.

It's easier to implicitely regard every individual use of an undefined SSA 
name as a unique name in coalescing I think (instead of having it be a 
unique name explicitely).  That is, given:

bb1:
  x_1 = PHI <a_2, b_3(UND)>
  ...

bb2:
  x_4 = PHI <y_5, b_3(UND)>
  ...

There is no reason to not regard the two uses of b_3 as separate and 
identify the first with x_1 and the second with x_2, _without_ coalescing 
x_1 and x_2.  But yes, this doesn't fit readily into the normal coalescing 
merge-find framework, but rather would have to be something after 
coalescing when writing out-of-ssa (whenever an undefined use is rewritten 
just take a random other fitting variable).


Ciao,
Michael.

Re: update address taken: don't drop clobbers

[Jeff Law]

On 07/11/14 06:05, Michael Matz wrote:
> Hi,
>
> On Thu, 10 Jul 2014, Jeff Law wrote:
>
>>> The insight to note is, that undefined SSA names should really be
>>> coalesced with something (otherwise you lost an optimization opportunity),
>>> but it doesn't matter with _what_ each use of the undefined name is
>>> coalesced, you can even identify different uses of them with different SSA
>>> names (e.g. the LHS of each using stmt).  Requires some change in the
>>> order things are done in out-of-ssa.
>>
>> The last part is what I hinted might be problematical.  If some
>> undefined SSA_NAME appears on the RHS of two PHIs and we want to
>> coalesce that undefined SSA_NAME with the LHS of each of those PHIs,
>> then the LHS of those two PHIs must coalesce as well.  At least that's
>> my recollection of how all that stuff worked.
>
> Only with the usual definition of coalescing (being transitive).  For
> undefined SSA names the definition can be mended.
My recollection is the transitive nature of coalescing is baked into our 
implementation.  I'd be happy to be proved wrong :-)


Jeff

Re: update address taken: don't drop clobbers

[Marc Glisse]

On Thu, 10 Jul 2014, Richard Biener wrote:

>> --- gcc/tree-into-ssa.c (revision 212109)
>> +++ gcc/tree-into-ssa.c (working copy)
>> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
>>  {
>>    tree def = DEF_FROM_PTR (def_p);
>>    tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>
>>    /* If DEF is a naked symbol that needs renaming, create a new
>>       name for it.  */
>>    if (marked_for_renaming (sym))
>>      {
>>        if (DECL_P (def))
>>         {
>> -         tree tracked_var;
>> -
>> -         def = make_ssa_name (def, stmt);
>> +         if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>
> sym should always be a gimple reg here (it's marked for renaming).
>
>> +           {
>> +             /* Replace clobber stmts with a default def.  Create a new
>> +                variable so we don't later think we must coalesce, which
>> would
>> +                fail with some ada abnormal PHIs.  Still, we try to keep a
>> +                similar name so error messages make sense.  */
>> +             unlink_stmt_vdef (stmt);
>
> I think that's redundant with gsi_replace (note that using gsi_replace
> looks dangerous here as it calls update_stmt during SSA rewrite...
> that might open a can of worms).

IIRC it was failing without unlink_stmt_vdef (maybe that was in a 
different version of the patch not using gsi_replace, but I don't think 
so). I was hoping that a clobber had little enough effects that 
update_stmt was unlikely to break anything. Anyway it doesn't matter if I 
use your suggestion below.

>> +             gsi_replace (&gsi, gimple_build_nop (), true);
>> +             tree id = DECL_NAME (sym);
>> +             const char* name = id ? IDENTIFIER_POINTER (id) : 0;
>> +             tree newvar = create_tmp_var (TREE_TYPE (sym), name);
>> +             def = get_or_create_ssa_default_def (cfun, newvar);
>
> So - can't you simply do
>
>    gimple_assign_set_rhs_from_tree (&gsi,
> get_or_create_dda_default_def (cfun, sym));
>
> ?  Thus replace x = CLOBBER; with x_3 = x_2(D);
>
>> +           }
>> +         else
>
> and of course still rewrite the DEF then.  IMHO the copy-propagation
> you do is premature optimization.

I'll try that. I was trying to remain as close as possible to what you 
wrote in rewrite_stmt:

         if (gimple_clobber_p (stmt)
             && is_gimple_reg (var))
           {
             /* If we rewrite a DECL into SSA form then drop its
                clobber stmts and replace uses with a new default def.  */
             gcc_checking_assert (TREE_CODE (var) == VAR_DECL
                                  && !gimple_vdef (stmt));
             gsi_replace (si, gimple_build_nop (), true);
             register_new_def (get_or_create_ssa_default_def (cfun, var), var);
             break;
           }


I'll be away next week, but I'll re-read all the replies carefully when I 
come back.

-- 
Marc Glisse

Re: update address taken: don't drop clobbers

[Richard Biener]

On Sat, Jul 12, 2014 at 8:15 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Thu, 10 Jul 2014, Richard Biener wrote:
>
>>> --- gcc/tree-into-ssa.c (revision 212109)
>>> +++ gcc/tree-into-ssa.c (working copy)
>>> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
>>>  {
>>>    tree def = DEF_FROM_PTR (def_p);
>>>    tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>>
>>>    /* If DEF is a naked symbol that needs renaming, create a new
>>>       name for it.  */
>>>    if (marked_for_renaming (sym))
>>>      {
>>>        if (DECL_P (def))
>>>         {
>>> -         tree tracked_var;
>>> -
>>> -         def = make_ssa_name (def, stmt);
>>> +         if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>>
>>
>> sym should always be a gimple reg here (it's marked for renaming).
>>
>>> +           {
>>> +             /* Replace clobber stmts with a default def.  Create a new
>>> +                variable so we don't later think we must coalesce, which
>>> would
>>> +                fail with some ada abnormal PHIs.  Still, we try to keep
>>> a
>>> +                similar name so error messages make sense.  */
>>> +             unlink_stmt_vdef (stmt);
>>
>>
>> I think that's redundant with gsi_replace (note that using gsi_replace
>> looks dangerous here as it calls update_stmt during SSA rewrite...
>> that might open a can of worms).
>
>
> IIRC it was failing without unlink_stmt_vdef (maybe that was in a different
> version of the patch not using gsi_replace, but I don't think so). I was
> hoping that a clobber had little enough effects that update_stmt was
> unlikely to break anything. Anyway it doesn't matter if I use your
> suggestion below.

The important part is that it not ICE (of course) and that it doesn't
trigger useless SSA renaming of .MEM - gsi_replace does
update_stmt which does the unlink_stmt_vdef for you if the .MEM
is no longer necessary.

Richard.

>
>>> +             gsi_replace (&gsi, gimple_build_nop (), true);
>>> +             tree id = DECL_NAME (sym);
>>> +             const char* name = id ? IDENTIFIER_POINTER (id) : 0;
>>> +             tree newvar = create_tmp_var (TREE_TYPE (sym), name);
>>> +             def = get_or_create_ssa_default_def (cfun, newvar);
>>
>>
>> So - can't you simply do
>>
>>    gimple_assign_set_rhs_from_tree (&gsi,
>> get_or_create_dda_default_def (cfun, sym));
>>
>> ?  Thus replace x = CLOBBER; with x_3 = x_2(D);
>>
>>> +           }
>>> +         else
>>
>>
>> and of course still rewrite the DEF then.  IMHO the copy-propagation
>> you do is premature optimization.
>
>
> I'll try that. I was trying to remain as close as possible to what you wrote
> in rewrite_stmt:
>
>         if (gimple_clobber_p (stmt)
>             && is_gimple_reg (var))
>           {
>             /* If we rewrite a DECL into SSA form then drop its
>                clobber stmts and replace uses with a new default def.  */
>             gcc_checking_assert (TREE_CODE (var) == VAR_DECL
>                                  && !gimple_vdef (stmt));
>             gsi_replace (si, gimple_build_nop (), true);
>             register_new_def (get_or_create_ssa_default_def (cfun, var),
> var);
>             break;
>           }
>
>
> I'll be away next week, but I'll re-read all the replies carefully when I
> come back.
>
> --
> Marc Glisse

Re: update address taken: don't drop clobbers

[Marc Glisse]

On Thu, 10 Jul 2014, Richard Biener wrote:

>> --- gcc/tree-into-ssa.c (revision 212109)
>> +++ gcc/tree-into-ssa.c (working copy)
>> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
>>  {
>>    tree def = DEF_FROM_PTR (def_p);
>>    tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>
>>    /* If DEF is a naked symbol that needs renaming, create a new
>>       name for it.  */
>>    if (marked_for_renaming (sym))
>>      {
>>        if (DECL_P (def))
>>         {
>> -         tree tracked_var;
>> -
>> -         def = make_ssa_name (def, stmt);
>> +         if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>
> sym should always be a gimple reg here (it's marked for renaming).

Not quite. If I remove the is_gimple_reg check, I get:

/data/repos/gcc/sra/libgcc/libgcc2.c: In function '__divti3':
/data/repos/gcc/sra/libgcc/libgcc2.c:1246:1: error: non-trivial conversion at assignment
  }
  ^
const union DWunion
void
# .MEM_149 = VDEF <.MEM_148>
nn ={v} .MEM_7(D);


>> +           {
>> +             /* Replace clobber stmts with a default def.  Create a new
>> +                variable so we don't later think we must coalesce, which
>> would
>> +                fail with some ada abnormal PHIs.  Still, we try to keep a
>> +                similar name so error messages make sense.  */
>> +             unlink_stmt_vdef (stmt);
>
> I think that's redundant with gsi_replace (note that using gsi_replace
> looks dangerous here as it calls update_stmt during SSA rewrite...
> that might open a can of worms).
>
>> +             gsi_replace (&gsi, gimple_build_nop (), true);
>> +             tree id = DECL_NAME (sym);
>> +             const char* name = id ? IDENTIFIER_POINTER (id) : 0;
>> +             tree newvar = create_tmp_var (TREE_TYPE (sym), name);
>> +             def = get_or_create_ssa_default_def (cfun, newvar);
>
> So - can't you simply do
>
>    gimple_assign_set_rhs_from_tree (&gsi,
> get_or_create_dda_default_def (cfun, sym));
>
> ?  Thus replace x = CLOBBER; with x_3 = x_2(D);

If I write just that, I get a failure because of a missing USE. I need to 
run update_stmt (but then you are saying it is dangerous...).

And it also fails to warn for the C++ testcase because SRA sets 
nowarning_flag (it doesn't if I create a new variable), but I guess we can 
see about changing that next.

-- 
Marc Glisse

Re: update address taken: don't drop clobbers

[Marc Glisse]

On Thu, 10 Jul 2014, Richard Biener wrote:

>> --- gcc/tree-into-ssa.c (revision 212109)
>> +++ gcc/tree-into-ssa.c (working copy)
>> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
>>  {
>>    tree def = DEF_FROM_PTR (def_p);
>>    tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>
>>    /* If DEF is a naked symbol that needs renaming, create a new
>>       name for it.  */
>>    if (marked_for_renaming (sym))
>>      {
>>        if (DECL_P (def))
>>         {
>> -         tree tracked_var;
>> -
>> -         def = make_ssa_name (def, stmt);
>> +         if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>
> sym should always be a gimple reg here (it's marked for renaming).
>
>> +           {
>> +             /* Replace clobber stmts with a default def.  Create a new
>> +                variable so we don't later think we must coalesce, which
>> would
>> +                fail with some ada abnormal PHIs.  Still, we try to keep a
>> +                similar name so error messages make sense.  */
>> +             unlink_stmt_vdef (stmt);
>
> I think that's redundant with gsi_replace (note that using gsi_replace
> looks dangerous here as it calls update_stmt during SSA rewrite...
> that might open a can of worms).
>
>> +             gsi_replace (&gsi, gimple_build_nop (), true);
>> +             tree id = DECL_NAME (sym);
>> +             const char* name = id ? IDENTIFIER_POINTER (id) : 0;
>> +             tree newvar = create_tmp_var (TREE_TYPE (sym), name);
>> +             def = get_or_create_ssa_default_def (cfun, newvar);
>
> So - can't you simply do
>
>    gimple_assign_set_rhs_from_tree (&gsi,
> get_or_create_dda_default_def (cfun, sym));
>
> ?  Thus replace x = CLOBBER; with x_3 = x_2(D);
>
>> +           }
>> +         else
>
> and of course still rewrite the DEF then.  IMHO the copy-propagation
> you do is premature optimization.

Using your version, I end up with spurious warnings, in particular for 
va_list. pass_fold_builtins stops va_start/va_end taking the address of 
the list, so we get:

   list_6 = list_2(D);

in place of the clobber at the end of the function. And there is no 
DCE-like pass afterwards, so we warn for the use of list_2(D).
(passes.def contains a comment about running dce before uninit)

I don't know if update_address_taken could avoid generating this 
assignment where the lhs has 0 use, but this shows the optimization is not 
completely premature.

(uninit could also check for this case, but that feels like a bad hack)

-- 
Marc Glisse

Re: update address taken: don't drop clobbers

[Marc Glisse]

On Sun, 27 Jul 2014, Marc Glisse wrote:

> On Thu, 10 Jul 2014, Richard Biener wrote:
>
>>> --- gcc/tree-into-ssa.c (revision 212109)
>>> +++ gcc/tree-into-ssa.c (working copy)
>>> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
>>>  {
>>>    tree def = DEF_FROM_PTR (def_p);
>>>    tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>>
>>>    /* If DEF is a naked symbol that needs renaming, create a new
>>>       name for it.  */
>>>    if (marked_for_renaming (sym))
>>>      {
>>>        if (DECL_P (def))
>>>         {
>>> -         tree tracked_var;
>>> -
>>> -         def = make_ssa_name (def, stmt);
>>> +         if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>> 
>> sym should always be a gimple reg here (it's marked for renaming).
>> 
>>> +           {
>>> +             /* Replace clobber stmts with a default def.  Create a new
>>> +                variable so we don't later think we must coalesce, which
>>> would
>>> +                fail with some ada abnormal PHIs.  Still, we try to keep 
>>> a
>>> +                similar name so error messages make sense.  */
>>> +             unlink_stmt_vdef (stmt);
>> 
>> I think that's redundant with gsi_replace (note that using gsi_replace
>> looks dangerous here as it calls update_stmt during SSA rewrite...
>> that might open a can of worms).
>> 
>>> +             gsi_replace (&gsi, gimple_build_nop (), true);
>>> +             tree id = DECL_NAME (sym);
>>> +             const char* name = id ? IDENTIFIER_POINTER (id) : 0;
>>> +             tree newvar = create_tmp_var (TREE_TYPE (sym), name);
>>> +             def = get_or_create_ssa_default_def (cfun, newvar);
>> 
>> So - can't you simply do
>>
>>    gimple_assign_set_rhs_from_tree (&gsi,
>> get_or_create_dda_default_def (cfun, sym));
>> 
>> ?  Thus replace x = CLOBBER; with x_3 = x_2(D);
>> 
>>> +           }
>>> +         else
>> 
>> and of course still rewrite the DEF then.  IMHO the copy-propagation
>> you do is premature optimization.
>
> Using your version, I end up with spurious warnings, in particular for 
> va_list. pass_fold_builtins stops va_start/va_end taking the address of the 
> list, so we get:
>
>  list_6 = list_2(D);
>
> in place of the clobber at the end of the function. And there is no DCE-like 
> pass afterwards, so we warn for the use of list_2(D).
> (passes.def contains a comment about running dce before uninit)
>
> I don't know if update_address_taken could avoid generating this assignment 
> where the lhs has 0 use, but this shows the optimization is not completely 
> premature.
>
> (uninit could also check for this case, but that feels like a bad hack)

I would like some guidance on this. I just tried this trivial patch:

        NEXT_PASS (pass_split_crit_edges);
+      NEXT_PASS (pass_dce);
        NEXT_PASS (pass_late_warn_uninitialized);

and it does not cause any regression, it even XPASS 
gfortran.dg/reassoc_6.f for some reason. The FIXME note just above in 
passes.def mentions 2 testcases that are already xfailed anyway.

Would that extra pass be acceptable?

Otherwise, what do you think should be responsible for cleaning up the 
dead assignments?

-- 
Marc Glisse

Re: update address taken: don't drop clobbers

[Marc Glisse]

On Sun, 7 Sep 2014, Marc Glisse wrote:

> On Sun, 27 Jul 2014, Marc Glisse wrote:
>
>> On Thu, 10 Jul 2014, Richard Biener wrote:
>> 
>>>> --- gcc/tree-into-ssa.c (revision 212109)
>>>> +++ gcc/tree-into-ssa.c (working copy)
>>>> @@ -1831,26 +1831,38 @@ maybe_register_def (def_operand_p def_p,
>>>>  {
>>>>    tree def = DEF_FROM_PTR (def_p);
>>>>    tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>>>
>>>>    /* If DEF is a naked symbol that needs renaming, create a new
>>>>       name for it.  */
>>>>    if (marked_for_renaming (sym))
>>>>      {
>>>>        if (DECL_P (def))
>>>>         {
>>>> -         tree tracked_var;
>>>> -
>>>> -         def = make_ssa_name (def, stmt);
>>>> +         if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>>> 
>>> sym should always be a gimple reg here (it's marked for renaming).
>>> 
>>>> +           {
>>>> +             /* Replace clobber stmts with a default def.  Create a new
>>>> +                variable so we don't later think we must coalesce, which
>>>> would
>>>> +                fail with some ada abnormal PHIs.  Still, we try to keep 
>>>> a
>>>> +                similar name so error messages make sense.  */
>>>> +             unlink_stmt_vdef (stmt);
>>> 
>>> I think that's redundant with gsi_replace (note that using gsi_replace
>>> looks dangerous here as it calls update_stmt during SSA rewrite...
>>> that might open a can of worms).
>>> 
>>>> +             gsi_replace (&gsi, gimple_build_nop (), true);
>>>> +             tree id = DECL_NAME (sym);
>>>> +             const char* name = id ? IDENTIFIER_POINTER (id) : 0;
>>>> +             tree newvar = create_tmp_var (TREE_TYPE (sym), name);
>>>> +             def = get_or_create_ssa_default_def (cfun, newvar);
>>> 
>>> So - can't you simply do
>>>
>>>    gimple_assign_set_rhs_from_tree (&gsi,
>>> get_or_create_dda_default_def (cfun, sym));
>>> 
>>> ?  Thus replace x = CLOBBER; with x_3 = x_2(D);
>>> 
>>>> +           }
>>>> +         else
>>> 
>>> and of course still rewrite the DEF then.  IMHO the copy-propagation
>>> you do is premature optimization.
>> 
>> Using your version, I end up with spurious warnings, in particular for 
>> va_list. pass_fold_builtins stops va_start/va_end taking the address of the 
>> list, so we get:
>>
>>  list_6 = list_2(D);
>> 
>> in place of the clobber at the end of the function. And there is no 
>> DCE-like pass afterwards, so we warn for the use of list_2(D).
>> (passes.def contains a comment about running dce before uninit)
>> 
>> I don't know if update_address_taken could avoid generating this assignment 
>> where the lhs has 0 use, but this shows the optimization is not completely 
>> premature.
>> 
>> (uninit could also check for this case, but that feels like a bad hack)
>
> I would like some guidance on this. I just tried this trivial patch:
>
>       NEXT_PASS (pass_split_crit_edges);
> +      NEXT_PASS (pass_dce);
>       NEXT_PASS (pass_late_warn_uninitialized);
>
> and it does not cause any regression, it even XPASS gfortran.dg/reassoc_6.f 
> for some reason. The FIXME note just above in passes.def mentions 2 testcases 
> that are already xfailed anyway.
>
> Would that extra pass be acceptable?
>
> Otherwise, what do you think should be responsible for cleaning up the dead 
> assignments?

Does anyone have an opinion on which side needs to be improved? As a 
reminder:

- we have a va_list with its address taken by va_start/va_end.
- fab lowers va_start/va_end and the list doesn't have its address taken 
anymore.
- update_address_taken replaces the clobber: list =v {}; with an 
assignment of an undefined value: list_6 = list_2(D);
- uninit warns about this.

Some possible directions:
- "prematurely" optimize in update_address_taken so we don't generate the 
useless assignment.
- add a dce pass before uninit.

-- 
Marc Glisse

Re: update address taken: don't drop clobbers

[Jeff Law]

On 10/15/14 08:35, Marc Glisse wrote:
>>
>> Would that extra pass be acceptable?
>>
>> Otherwise, what do you think should be responsible for cleaning up the
>> dead assignments?
>
> Does anyone have an opinion on which side needs to be improved? As a
> reminder:
>
> - we have a va_list with its address taken by va_start/va_end.
> - fab lowers va_start/va_end and the list doesn't have its address taken
> anymore.
> - update_address_taken replaces the clobber: list =v {}; with an
> assignment of an undefined value: list_6 = list_2(D);
> - uninit warns about this.
>
> Some possible directions:
> - "prematurely" optimize in update_address_taken so we don't generate
> the useless assignment.
> - add a dce pass before uninit.
I tend to land on the side of minimizing false positives, so the comment 
about PR18501 is a "don't care" to me.  If the optimizers remove a dead 
assignment and we no longer warn about a potential uninitialized use in 
the dead assignment, then I consider that good.  Not everyone agrees 
with that way of thinking, obviously.

So my inclination would be to evaluate independent of the pr18501 
issues.  ie, what's the compile-time cost vs runtime benefit of running 
DCE here.  I'm guessing there's little runtime benefit for this 
particular case.

So my next line of thinking would be can we arrange to conditionally run 
DCE?  ie, have update_address_taken signal that it did something that 
has a reasonable chance of exposing dead code and only run DCE in that 
case.  Obviously this only helps if it rarely signals :-)  I don't think 
we have any infrastructure for this right now.

Finally I'd look at how difficult it would be to have 
update_address_taken cleanup after itself.   If the LHS is in SSA form, 
then if we find it has no uses, can we just remove the assignment 
completely?

jeff

Re: update address taken: don't drop clobbers

[Richard Biener]

On Wed, Oct 15, 2014 at 6:08 PM, Jeff Law <law@redhat.com> wrote:
> On 10/15/14 08:35, Marc Glisse wrote:
>>>
>>>
>>> Would that extra pass be acceptable?

Ugh, rather not.  We have too many passes ;)

>>> Otherwise, what do you think should be responsible for cleaning up the
>>> dead assignments?
>>
>>
>> Does anyone have an opinion on which side needs to be improved? As a
>> reminder:
>>
>> - we have a va_list with its address taken by va_start/va_end.
>> - fab lowers va_start/va_end and the list doesn't have its address taken
>> anymore.
>> - update_address_taken replaces the clobber: list =v {}; with an
>> assignment of an undefined value: list_6 = list_2(D);
>> - uninit warns about this.
>>
>> Some possible directions:
>> - "prematurely" optimize in update_address_taken so we don't generate
>> the useless assignment.
>> - add a dce pass before uninit.
>
> I tend to land on the side of minimizing false positives, so the comment
> about PR18501 is a "don't care" to me.  If the optimizers remove a dead
> assignment and we no longer warn about a potential uninitialized use in the
> dead assignment, then I consider that good.  Not everyone agrees with that
> way of thinking, obviously.
>
> So my inclination would be to evaluate independent of the pr18501 issues.
> ie, what's the compile-time cost vs runtime benefit of running DCE here.
> I'm guessing there's little runtime benefit for this particular case.
>
> So my next line of thinking would be can we arrange to conditionally run
> DCE?  ie, have update_address_taken signal that it did something that has a
> reasonable chance of exposing dead code and only run DCE in that case.
> Obviously this only helps if it rarely signals :-)  I don't think we have
> any infrastructure for this right now.
>
> Finally I'd look at how difficult it would be to have update_address_taken
> cleanup after itself.   If the LHS is in SSA form, then if we find it has no
> uses, can we just remove the assignment completely?

It doesn't even know that it has no uses (the variable still needs to be
written into SSA form).  OTOH it is a missed DSE opportunity before
update-address-taken?

As of premature optimization - into-SSA could notice it created SSA
names with no uses and trigger a fast DCE.

Btw, I wonder what this odd folding of variadic builtins is about, and why
it is not done in the stdarg pass (and only there), which would be earlier.

Richard.

> jeff

Re: update address taken: don't drop clobbers

[Marc Glisse]

On Thu, 16 Oct 2014, Richard Biener wrote:

> On Wed, Oct 15, 2014 at 6:08 PM, Jeff Law <law@redhat.com> wrote:
>> On 10/15/14 08:35, Marc Glisse wrote:
>>>>
>>>>
>>>> Would that extra pass be acceptable?
>
> Ugh, rather not.  We have too many passes ;)

I expected you would say that...

>>>> Otherwise, what do you think should be responsible for cleaning up the
>>>> dead assignments?
>>>
>>>
>>> Does anyone have an opinion on which side needs to be improved? As a
>>> reminder:
>>>
>>> - we have a va_list with its address taken by va_start/va_end.
>>> - fab lowers va_start/va_end and the list doesn't have its address taken
>>> anymore.
>>> - update_address_taken replaces the clobber: list =v {}; with an
>>> assignment of an undefined value: list_6 = list_2(D);
>>> - uninit warns about this.
>>>
>>> Some possible directions:
>>> - "prematurely" optimize in update_address_taken so we don't generate
>>> the useless assignment.
>>> - add a dce pass before uninit.
>>
>> I tend to land on the side of minimizing false positives, so the comment
>> about PR18501 is a "don't care" to me.  If the optimizers remove a dead
>> assignment and we no longer warn about a potential uninitialized use in the
>> dead assignment, then I consider that good.  Not everyone agrees with that
>> way of thinking, obviously.

I agree with that.

>> So my inclination would be to evaluate independent of the pr18501 issues.
>> ie, what's the compile-time cost vs runtime benefit of running DCE here.
>> I'm guessing there's little runtime benefit for this particular case.
>>
>> So my next line of thinking would be can we arrange to conditionally run
>> DCE?  ie, have update_address_taken signal that it did something that has a
>> reasonable chance of exposing dead code and only run DCE in that case.
>> Obviously this only helps if it rarely signals :-)  I don't think we have
>> any infrastructure for this right now.
>>
>> Finally I'd look at how difficult it would be to have update_address_taken
>> cleanup after itself.   If the LHS is in SSA form, then if we find it has no
>> uses, can we just remove the assignment completely?
>
> It doesn't even know that it has no uses (the variable still needs to be
> written into SSA form).  OTOH it is a missed DSE opportunity before
> update-address-taken?

Uh, we are talking about what happens to a clobber when the variable stops 
having its address taken. We don't want to DSE clobbers (do we?).

(and va_start is just an example, many transformations could cause this)

> As of premature optimization - into-SSA could notice it created SSA
> names with no uses and trigger a fast DCE.

I am looking into that, it must be doable. It seems not too hard, in 
maybe_register_def, to push all results from make_ssa_name to some 
data-structure (I don't think new_ssa_names gives me that list, but there 
may be other ways to get it without introducing yet another list), and 
either mark them as used in maybe_replace_use or get_reaching_def, or 
better loop through them at the end, checking has_zero_uses (it is a bit 
wasteful, only those coming from clobbers may have 0 uses (or we missed a 
dce/dse earlier), but it should be fast enough, even walking on all 
ssa_names should be fast enough). If we go to that much trouble, we may as 
well clean them while we are there. It isn't obvious to me how to notice 
unused new ssa_names more easily, to trigger a DCE.

> Btw, I wonder what this odd folding of variadic builtins is about, and why
> it is not done in the stdarg pass (and only there), which would be earlier.

pass_fold_builtins::execute has:

                 case BUILT_IN_VA_START:
                 case BUILT_IN_VA_END:
                 case BUILT_IN_VA_COPY:
                   /* These shouldn't be folded before pass_stdarg.  */
                   result = optimize_stdarg_builtin (stmt);

while pass_stdarg only mentions them as:

           /* Don't look at __builtin_va_{start,end}, they are ok.  */

It does seem late, but that will be for someone else to look at.


By the way, I noticed something strange in tree-into-ssa.c:

static inline bool
is_old_name (tree name)
{
   unsigned ver = SSA_NAME_VERSION (name);
   if (!new_ssa_names)
     return false;
   return (ver < SBITMAP_SIZE (new_ssa_names)
           && bitmap_bit_p (old_ssa_names, ver));
}

There are a lot of "new" in this function about "old".

-- 
Marc Glisse

Re: update address taken: don't drop clobbers

[Richard Biener]

On Thu, Oct 16, 2014 at 4:11 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Thu, 16 Oct 2014, Richard Biener wrote:
>
>> On Wed, Oct 15, 2014 at 6:08 PM, Jeff Law <law@redhat.com> wrote:
>>>
>>> On 10/15/14 08:35, Marc Glisse wrote:
>>>>>
>>>>>
>>>>>
>>>>> Would that extra pass be acceptable?
>>
>>
>> Ugh, rather not.  We have too many passes ;)
>
>
> I expected you would say that...
>
>>>>> Otherwise, what do you think should be responsible for cleaning up the
>>>>> dead assignments?
>>>>
>>>>
>>>>
>>>> Does anyone have an opinion on which side needs to be improved? As a
>>>> reminder:
>>>>
>>>> - we have a va_list with its address taken by va_start/va_end.
>>>> - fab lowers va_start/va_end and the list doesn't have its address taken
>>>> anymore.
>>>> - update_address_taken replaces the clobber: list =v {}; with an
>>>> assignment of an undefined value: list_6 = list_2(D);
>>>> - uninit warns about this.
>>>>
>>>> Some possible directions:
>>>> - "prematurely" optimize in update_address_taken so we don't generate
>>>> the useless assignment.
>>>> - add a dce pass before uninit.
>>>
>>>
>>> I tend to land on the side of minimizing false positives, so the comment
>>> about PR18501 is a "don't care" to me.  If the optimizers remove a dead
>>> assignment and we no longer warn about a potential uninitialized use in
>>> the
>>> dead assignment, then I consider that good.  Not everyone agrees with
>>> that
>>> way of thinking, obviously.
>
>
> I agree with that.
>
>>> So my inclination would be to evaluate independent of the pr18501 issues.
>>> ie, what's the compile-time cost vs runtime benefit of running DCE here.
>>> I'm guessing there's little runtime benefit for this particular case.
>>>
>>> So my next line of thinking would be can we arrange to conditionally run
>>> DCE?  ie, have update_address_taken signal that it did something that has
>>> a
>>> reasonable chance of exposing dead code and only run DCE in that case.
>>> Obviously this only helps if it rarely signals :-)  I don't think we have
>>> any infrastructure for this right now.
>>>
>>> Finally I'd look at how difficult it would be to have
>>> update_address_taken
>>> cleanup after itself.   If the LHS is in SSA form, then if we find it has
>>> no
>>> uses, can we just remove the assignment completely?
>>
>>
>> It doesn't even know that it has no uses (the variable still needs to be
>> written into SSA form).  OTOH it is a missed DSE opportunity before
>> update-address-taken?
>
>
> Uh, we are talking about what happens to a clobber when the variable stops
> having its address taken. We don't want to DSE clobbers (do we?).

No.

> (and va_start is just an example, many transformations could cause this)

Ok, I see.

>> As of premature optimization - into-SSA could notice it created SSA
>> names with no uses and trigger a fast DCE.
>
>
> I am looking into that, it must be doable. It seems not too hard, in
> maybe_register_def, to push all results from make_ssa_name to some
> data-structure (I don't think new_ssa_names gives me that list, but there
> may be other ways to get it without introducing yet another list), and
> either mark them as used in maybe_replace_use or get_reaching_def, or better
> loop through them at the end, checking has_zero_uses (it is a bit wasteful,
> only those coming from clobbers may have 0 uses (or we missed a dce/dse
> earlier), but it should be fast enough, even walking on all ssa_names should
> be fast enough). If we go to that much trouble, we may as well clean them
> while we are there. It isn't obvious to me how to notice unused new
> ssa_names more easily, to trigger a DCE.

Yeah, well.  DCE should in theory also be fast (but it got too many features
so it isn't really fast - maybe guard some stuff in it).

>> Btw, I wonder what this odd folding of variadic builtins is about, and why
>> it is not done in the stdarg pass (and only there), which would be
>> earlier.
>
>
> pass_fold_builtins::execute has:
>
>                 case BUILT_IN_VA_START:
>                 case BUILT_IN_VA_END:
>                 case BUILT_IN_VA_COPY:
>                   /* These shouldn't be folded before pass_stdarg.  */
>                   result = optimize_stdarg_builtin (stmt);
>
> while pass_stdarg only mentions them as:
>
>           /* Don't look at __builtin_va_{start,end}, they are ok.  */
>
> It does seem late, but that will be for someone else to look at.
>
>
> By the way, I noticed something strange in tree-into-ssa.c:
>
> static inline bool
> is_old_name (tree name)
> {
>   unsigned ver = SSA_NAME_VERSION (name);
>   if (!new_ssa_names)
>     return false;
>   return (ver < SBITMAP_SIZE (new_ssa_names)
>           && bitmap_bit_p (old_ssa_names, ver));
> }
>
> There are a lot of "new" in this function about "old".

Eh - indeed.  Patch to fix that pre-approved.

Richard.

>
> --
> Marc Glisse

Re: update address taken: don't drop clobbers

[Jeff Law]

On 10/16/14 08:11, Marc Glisse wrote:

>
> I am looking into that, it must be doable. It seems not too hard, in
> maybe_register_def, to push all results from make_ssa_name to some
> data-structure (I don't think new_ssa_names gives me that list, but
> there may be other ways to get it without introducing yet another list),
> and either mark them as used in maybe_replace_use or get_reaching_def,
> or better loop through them at the end, checking has_zero_uses (it is a
> bit wasteful, only those coming from clobbers may have 0 uses (or we
> missed a dce/dse earlier), but it should be fast enough, even walking on
> all ssa_names should be fast enough). If we go to that much trouble, we
> may as well clean them while we are there. It isn't obvious to me how to
> notice unused new ssa_names more easily, to trigger a DCE.
I'd walk the SSA_NAMEs at the end checking for zero uses.  I'm curious 
how often that will trigger  :-)

jeff

Re: update address taken: don't drop clobbers

[Richard Biener]

On October 16, 2014 7:26:48 PM CEST, Jeff Law <law@redhat.com> wrote:
>On 10/16/14 08:11, Marc Glisse wrote:
>
>>
>> I am looking into that, it must be doable. It seems not too hard, in
>> maybe_register_def, to push all results from make_ssa_name to some
>> data-structure (I don't think new_ssa_names gives me that list, but
>> there may be other ways to get it without introducing yet another
>list),
>> and either mark them as used in maybe_replace_use or
>get_reaching_def,
>> or better loop through them at the end, checking has_zero_uses (it is
>a
>> bit wasteful, only those coming from clobbers may have 0 uses (or we
>> missed a dce/dse earlier), but it should be fast enough, even walking
>on
>> all ssa_names should be fast enough). If we go to that much trouble,
>we
>> may as well clean them while we are there. It isn't obvious to me how
>to
>> notice unused new ssa_names more easily, to trigger a DCE.
>I'd walk the SSA_NAMEs at the end checking for zero uses.  I'm curious 
>how often that will trigger  :-)

Most often for the initial into SSA I guess. After that only for the cases we rename a variable which does not happen often. SRA and update-address-taken cone to my mind.

BTW, I dislike having multiple DCE implementations...

Richard.

>jeff

Re: update address taken: don't drop clobbers

[Jeff Law]

On 10/16/14 11:52, Richard Biener wrote:
>> I'd walk the SSA_NAMEs at the end checking for zero uses.  I'm
>> curious how often that will trigger  :-)
>
> Most often for the initial into SSA I guess. After that only for the
> cases we rename a variable which does not happen often. SRA and
> update-address-taken cone to my mind.
That'd be my guess too.  Jump threading probably creates them as well 
since we don't try to find any of the code that becomes dead in the 
duplicates after we remove the conditional.  Which means after DOM and VRP.

I don't suppose we have a map of newly created names handy anywhere :-)


>
> BTW, I dislike having multiple DCE implementations...
Similarly.  The proposal above was just to determine if we should 
schedule DCE or not.

jeff

Re: update address taken: don't drop clobbers

[Marc Glisse]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 724 bytes --]

On Thu, 16 Oct 2014, Jeff Law wrote:

>> BTW, I dislike having multiple DCE implementations...
> Similarly.  The proposal above was just to determine if we should schedule 
> DCE or not.

Thinking about it some more, I don't think we should need any kind of DCE 
here. The rewriting in update_ssa already does a form of forward 
propagation that avoids generating dead assignments, the problem only 
occurs if we explicitly introduce this new assignment. So I believe we 
should go back to an earlier version, like the attached, which is less 
work for the compiler.

And now I can go re-read the old discussion (apparently I should avoid 
gsi_replace, and there may be other ways to handle the coalescing).

-- 
Marc Glisse

[-- Attachment #2: Type: TEXT/PLAIN, Size: 8326 bytes --]

Index: testsuite/gcc.dg/tree-ssa/pr60770-1.c
===================================================================
--- testsuite/gcc.dg/tree-ssa/pr60770-1.c	(revision 0)
+++ testsuite/gcc.dg/tree-ssa/pr60770-1.c	(working copy)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O -Wall" } */
+
+int f(int n){
+  int*p;
+  {
+    int yyy=n;
+    p=&yyy;
+  }
+  return *p; /* { dg-warning "yyy" } */
+}
Index: tree-into-ssa.c
===================================================================
--- tree-into-ssa.c	(revision 216384)
+++ tree-into-ssa.c	(working copy)
@@ -1837,26 +1837,35 @@ maybe_register_def (def_operand_p def_p,
 {
   tree def = DEF_FROM_PTR (def_p);
   tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
 
   /* If DEF is a naked symbol that needs renaming, create a new
      name for it.  */
   if (marked_for_renaming (sym))
     {
       if (DECL_P (def))
 	{
-	  tree tracked_var;
-
-	  def = make_ssa_name (def, stmt);
+	  if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
+	    {
+	      /* Replace clobber stmts with a default def. This new use of a
+		 default definition may make it look like SSA_NAMEs have
+		 conflicting lifetimes, so we need special code to let them
+		 coalesce properly.  */
+	      unlink_stmt_vdef (stmt);
+	      gsi_replace (&gsi, gimple_build_nop (), true);
+	      def = get_or_create_ssa_default_def (cfun, sym);
+	    }
+	  else
+	    def = make_ssa_name (def, stmt);
 	  SET_DEF (def_p, def);
 
-	  tracked_var = target_for_debug_bind (sym);
+	  tree tracked_var = target_for_debug_bind (sym);
 	  if (tracked_var)
 	    {
 	      gimple note = gimple_build_debug_bind (tracked_var, def, stmt);
 	      /* If stmt ends the bb, insert the debug stmt on the single
 		 non-EH edge from the stmt.  */
 	      if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
 		{
 		  basic_block bb = gsi_bb (gsi);
 		  edge_iterator ei;
 		  edge e, ef = NULL;
Index: tree-ssa-live.c
===================================================================
--- tree-ssa-live.c	(revision 216384)
+++ tree-ssa-live.c	(working copy)
@@ -40,20 +40,21 @@ along with GCC; see the file COPYING3.
 #include "stringpool.h"
 #include "tree-ssanames.h"
 #include "expr.h"
 #include "tree-dfa.h"
 #include "timevar.h"
 #include "dumpfile.h"
 #include "tree-ssa-live.h"
 #include "diagnostic-core.h"
 #include "debug.h"
 #include "flags.h"
+#include "tree-ssa.h"
 
 #ifdef ENABLE_CHECKING
 static void  verify_live_on_entry (tree_live_info_p);
 #endif
 
 
 /* VARMAP maintains a mapping from SSA version number to real variables.
 
    All SSA_NAMES are divided into partitions.  Initially each ssa_name is the
    only member of it's own partition.  Coalescing will attempt to group any
@@ -1086,20 +1087,24 @@ set_var_live_on_entry (tree ssa_name, tr
   if (stmt)
     {
       def_bb = gimple_bb (stmt);
       /* Mark defs in liveout bitmap temporarily.  */
       if (def_bb)
 	bitmap_set_bit (&live->liveout[def_bb->index], p);
     }
   else
     def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
 
+  /* An undefined local variable does not need to be very alive.  */
+  if (ssa_undefined_value_p (ssa_name, false))
+    return;
+
   /* Visit each use of SSA_NAME and if it isn't in the same block as the def,
      add it to the list of live on entry blocks.  */
   FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
     {
       gimple use_stmt = USE_STMT (use);
       basic_block add_block = NULL;
 
       if (gimple_code (use_stmt) == GIMPLE_PHI)
         {
 	  /* Uses in PHI's are considered to be live at exit of the SRC block
@@ -1422,20 +1427,25 @@ verify_live_on_entry (tree_live_info_p l
 			  fprintf (stderr, "\n");
 			}
 		      else
 			fprintf (stderr, " and there is no default def.\n");
 		    }
 		}
 	    }
 	  else
 	    if (d == var)
 	      {
+		/* An undefined local variable does not need to be very
+		   alive.  */
+		if (ssa_undefined_value_p (var, false))
+		  continue;
+
 		/* The only way this var shouldn't be marked live on entry is
 		   if it occurs in a PHI argument of the block.  */
 		size_t z;
 		bool ok = false;
 		gimple_stmt_iterator gsi;
 		for (gsi = gsi_start_phis (e->dest);
 		     !gsi_end_p (gsi) && !ok;
 		     gsi_next (&gsi))
 		  {
 		    gimple phi = gsi_stmt (gsi);
Index: tree-ssa.c
===================================================================
--- tree-ssa.c	(revision 216384)
+++ tree-ssa.c	(working copy)
@@ -1178,24 +1178,25 @@ tree_ssa_useless_type_conversion (tree e
 
 tree
 tree_ssa_strip_useless_type_conversions (tree exp)
 {
   while (tree_ssa_useless_type_conversion (exp))
     exp = TREE_OPERAND (exp, 0);
   return exp;
 }
 
 
-/* Return true if T, an SSA_NAME, has an undefined value.  */
+/* Return true if T, an SSA_NAME, has an undefined value.  PARTIAL is what
+   should be returned if the value is only partially undefined.  */
 
 bool
-ssa_undefined_value_p (tree t)
+ssa_undefined_value_p (tree t, bool partial)
 {
   gimple def_stmt;
   tree var = SSA_NAME_VAR (t);
 
   if (!var)
     ;
   /* Parameters get their initial value from the function entry.  */
   else if (TREE_CODE (var) == PARM_DECL)
     return false;
   /* When returning by reference the return address is actually a hidden
@@ -1205,21 +1206,21 @@ ssa_undefined_value_p (tree t)
   /* Hard register variables get their initial value from the ether.  */
   else if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var))
     return false;
 
   /* The value is undefined iff its definition statement is empty.  */
   def_stmt = SSA_NAME_DEF_STMT (t);
   if (gimple_nop_p (def_stmt))
     return true;
 
   /* Check if the complex was not only partially defined.  */
-  if (is_gimple_assign (def_stmt)
+  if (partial && is_gimple_assign (def_stmt)
       && gimple_assign_rhs_code (def_stmt) == COMPLEX_EXPR)
     {
       tree rhs1, rhs2;
 
       rhs1 = gimple_assign_rhs1 (def_stmt);
       rhs2 = gimple_assign_rhs2 (def_stmt);
       return (TREE_CODE (rhs1) == SSA_NAME && ssa_undefined_value_p (rhs1))
 	     || (TREE_CODE (rhs2) == SSA_NAME && ssa_undefined_value_p (rhs2));
     }
   return false;
@@ -1551,32 +1552,20 @@ execute_update_addresses_taken (void)
 		rhs = gimple_assign_rhs1 (stmt);
 		if (gimple_assign_lhs (stmt) != lhs
 		    && !useless_type_conversion_p (TREE_TYPE (lhs),
 						   TREE_TYPE (rhs)))
 		  rhs = fold_build1 (VIEW_CONVERT_EXPR,
 				     TREE_TYPE (lhs), rhs);
 
 		if (gimple_assign_lhs (stmt) != lhs)
 		  gimple_assign_set_lhs (stmt, lhs);
 
-		/* For var ={v} {CLOBBER}; where var lost
-		   TREE_ADDRESSABLE just remove the stmt.  */
-		if (DECL_P (lhs)
-		    && TREE_CLOBBER_P (rhs)
-		    && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
-		  {
-		    unlink_stmt_vdef (stmt);
-      		    gsi_remove (&gsi, true);
-		    release_defs (stmt);
-		    continue;
-		  }
-
 		if (gimple_assign_rhs1 (stmt) != rhs)
 		  {
 		    gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
 		    gimple_assign_set_rhs_from_tree (&gsi, rhs);
 		  }
 	      }
 
 	    else if (gimple_code (stmt) == GIMPLE_CALL)
 	      {
 		unsigned i;
Index: tree-ssa.h
===================================================================
--- tree-ssa.h	(revision 216384)
+++ tree-ssa.h	(working copy)
@@ -44,21 +44,21 @@ extern tree target_for_debug_bind (tree)
 extern void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
 extern void insert_debug_temps_for_defs (gimple_stmt_iterator *);
 extern void reset_debug_uses (gimple);
 extern void release_defs_bitset (bitmap toremove);
 extern void verify_ssa (bool, bool);
 extern void init_tree_ssa (struct function *);
 extern void delete_tree_ssa (void);
 extern bool tree_ssa_useless_type_conversion (tree);
 extern tree tree_ssa_strip_useless_type_conversions (tree);
 
-extern bool ssa_undefined_value_p (tree);
+extern bool ssa_undefined_value_p (tree, bool = true);
 extern void execute_update_addresses_taken (void);
 
 /* Given an edge_var_map V, return the PHI arg definition.  */
 
 static inline tree
 redirect_edge_var_map_def (edge_var_map *v)
 {
   return v->def;
 }
 

Re: update address taken: don't drop clobbers

[Jeff Law]

On 10/17/14 14:41, Marc Glisse wrote:
> On Thu, 16 Oct 2014, Jeff Law wrote:
>
>>> BTW, I dislike having multiple DCE implementations...
>> Similarly.  The proposal above was just to determine if we should
>> schedule DCE or not.
>
> Thinking about it some more, I don't think we should need any kind of
> DCE here. The rewriting in update_ssa already does a form of forward
> propagation that avoids generating dead assignments, the problem only
> occurs if we explicitly introduce this new assignment. So I believe we
> should go back to an earlier version, like the attached, which is less
> work for the compiler.
>
> And now I can go re-read the old discussion (apparently I should avoid
> gsi_replace, and there may be other ways to handle the coalescing).
>
I'm starting to agree -- a later message indicated you wanted to drop 
the unlink_stmt_vdef call and you wanted to avoid gsi_replace, that 
seems fine.  I'll approve once those things are taken care of.

jeff

Re: update address taken: don't drop clobbers

[Marc Glisse]

(replying to both messages)

On Fri, 24 Oct 2014, Jeff Law wrote:

[ https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01830.html ]
> So I'm still trying to get comfortable with this patch.  I guess my concerns 
> about having one of the undefined value SSA_NAMEs appearing in two 
> conflicting coalesce lists are alleviated by the twiddle to 
> coalesce_partitions where we essentially ignore them.
>
> So in the end, they don't end up a part of any partition?

Without the liveness patch, their lifetime should mean that they don't 
coalesce with anything. But I would expect they get their own partition 
then (I am forgetting these details way too fast...). With the liveness 
patch which gives them an empty lifetime, they should all coalesce with at 
least one other ssa_name.

> What happens when we expand them?  I guess they get a new pseudo since 
> they're a distinct partition?  If we had a sensible story for expansion, 
> then I could probably get on board with this patch.

As I mentioned in my message, I don't know. Last time I looked I couldn't 
find how coalescing was actually performed. tree-outof-ssa.c has a 
function rewrite_trees with a promising comment but an empty body :-/
I agree that we need to understand what happens at expansion time when the 
variables are not coalesced before pushing a patch that prevents 
coalescing. I was kind of hoping someone would have a pointer...


On Fri, 24 Oct 2014, Jeff Law wrote:

[ https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html ]
> On 10/17/14 14:41, Marc Glisse wrote:
>> 
>> Thinking about it some more, I don't think we should need any kind of
>> DCE here. The rewriting in update_ssa already does a form of forward
>> propagation that avoids generating dead assignments, the problem only
>> occurs if we explicitly introduce this new assignment. So I believe we
>> should go back to an earlier version, like the attached, which is less
>> work for the compiler.
>> 
>> And now I can go re-read the old discussion (apparently I should avoid
>> gsi_replace, and there may be other ways to handle the coalescing).
>> 
> I'm starting to agree -- a later message indicated you wanted to drop the 
> unlink_stmt_vdef call and you wanted to avoid gsi_replace, that seems fine. 
> I'll approve once those things are taken care of.

I don't really want to avoid gsi_replace, but I am willing to do it if it 
makes reviewers nervous to call such a high-level function in the cleanup 
code between passes (Richard in particular was unhappy about it).

To clarify things so I know what to test and re-post, we are talking 
about the patch in 
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html and we can forget 
about the coalescing thing in 
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01830.html ?
(I'd be happy with that :-)

Thanks,

-- 
Marc Glisse

Re: update address taken: don't drop clobbers

[Jeff Law]

On 10/25/14 01:48, Marc Glisse wrote:
>
> Without the liveness patch, their lifetime should mean that they don't
> coalesce with anything. But I would expect they get their own partition
> then (I am forgetting these details way too fast...). With the liveness
> patch which gives them an empty lifetime, they should all coalesce with
> at least one other ssa_name.
>
>> What happens when we expand them?  I guess they get a new pseudo since
>> they're a distinct partition?  If we had a sensible story for
>> expansion, then I could probably get on board with this patch.
>
> As I mentioned in my message, I don't know. Last time I looked I
> couldn't find how coalescing was actually performed. tree-outof-ssa.c
> has a function rewrite_trees with a promising comment but an empty body :-/
> I agree that we need to understand what happens at expansion time when
> the variables are not coalesced before pushing a patch that prevents
> coalescing. I was kind of hoping someone would have a pointer...
Matz changed all the out-of-ssa stuff a while back.  Basically instead 
of actually rewriting things to reflect going out of SSA, we just record 
the partitions and refer back to the partitions representative element 
during expansion.

So all we really need to do is verify reasonable partitions are built 
and that should answer any questions around expansion.


> I don't really want to avoid gsi_replace, but I am willing to do it if
> it makes reviewers nervous to call such a high-level function in the
> cleanup code between passes (Richard in particular was unhappy about it).
Ah, I thought it was something you'd wanted, but sounds like it was 
Richi's request.


>
> To clarify things so I know what to test and re-post, we are talking
> about the patch in
> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html and we can
> forget about the coalescing thing in
> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01830.html ?
> (I'd be happy with that :-)
Yes, those are the ones I was referring to.

jeff

Re: update address taken: don't drop clobbers

[Marc Glisse]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1187 bytes --]

On Fri, 24 Oct 2014, Jeff Law wrote:

> I'm starting to agree -- a later message indicated you wanted to drop the 
> unlink_stmt_vdef call and you wanted to avoid gsi_replace, that seems fine. 
> I'll approve once those things are taken care of.

The following passed bootstrap+testsuite. I wasn't sure what exactly a 
clobber is guaranteed not to have (no histograms for instance? At least I 
assumed it doesn't throw) so I may have kept some unnecessary calls when I 
inlined gsi_replace. I'd be happy to remove any you feel is useless.

2014-10-26  Marc Glisse  <marc.glisse@inria.fr>

 	PR tree-optimization/60770
gcc/
 	* tree-into-ssa.c: Include value-prof.h.
 	(maybe_register_def): Replace clobbers with default definitions.
 	* tree-ssa-live.c: Include tree-ssa.h.
 	(set_var_live_on_entry): Do not mark undefined variables as live.
 	(verify_live_on_entry): Do not check undefined variables.
 	* tree-ssa.h (ssa_undefined_value_p): New parameter for the case
 	of partially undefined variables.
 	* tree-ssa.c (ssa_undefined_value_p): Likewise.
 	(execute_update_addresses_taken): Do not drop clobbers.
gcc/testsuite/
 	* gcc.dg/tree-ssa/pr60770-1.c: New file.

-- 
Marc Glisse

[-- Attachment #2: Type: TEXT/PLAIN, Size: 9455 bytes --]

Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c	(working copy)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O -Wall" } */
+
+int f(int n){
+  int*p;
+  {
+    int yyy=n;
+    p=&yyy;
+  }
+  return *p; /* { dg-warning "yyy" } */
+}
Index: gcc/tree-into-ssa.c
===================================================================
--- gcc/tree-into-ssa.c	(revision 216689)
+++ gcc/tree-into-ssa.c	(working copy)
@@ -52,20 +52,21 @@ along with GCC; see the file COPYING3.
 #include "expr.h"
 #include "tree-dfa.h"
 #include "tree-ssa.h"
 #include "tree-inline.h"
 #include "tree-pass.h"
 #include "cfgloop.h"
 #include "domwalk.h"
 #include "params.h"
 #include "diagnostic-core.h"
 #include "tree-into-ssa.h"
+#include "value-prof.h"
 
 #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y))
 
 /* This file builds the SSA form for a function as described in:
    R. Cytron, J. Ferrante, B. Rosen, M. Wegman, and K. Zadeck. Efficiently
    Computing Static Single Assignment Form and the Control Dependence
    Graph. ACM Transactions on Programming Languages and Systems,
    13(4):451-490, October 1991.  */
 
 /* Structure to map a variable VAR to the set of blocks that contain
@@ -1837,26 +1838,42 @@ maybe_register_def (def_operand_p def_p,
 {
   tree def = DEF_FROM_PTR (def_p);
   tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
 
   /* If DEF is a naked symbol that needs renaming, create a new
      name for it.  */
   if (marked_for_renaming (sym))
     {
       if (DECL_P (def))
 	{
-	  tree tracked_var;
+	  if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
+	    {
+	      /* Replace clobber stmts with a default def. This new use of a
+		 default definition may make it look like SSA_NAMEs have
+		 conflicting lifetimes, so we need special code to let them
+		 coalesce properly.  */
+	      /* Hand-inlined version of the following, for safety
+		 gsi_replace (&gsi, gimple_build_nop (), true);  */
+	      gimple nop = gimple_build_nop ();
+	      gimple_set_bb (nop, gsi_bb (gsi));
+	      gimple_set_bb (stmt, NULL);
+	      gimple_remove_stmt_histograms (cfun, stmt);
+	      delink_stmt_imm_use (stmt);
+	      gsi_set_stmt (&gsi, nop);
 
-	  def = make_ssa_name (def, stmt);
+	      def = get_or_create_ssa_default_def (cfun, sym);
+	    }
+	  else
+	    def = make_ssa_name (def, stmt);
 	  SET_DEF (def_p, def);
 
-	  tracked_var = target_for_debug_bind (sym);
+	  tree tracked_var = target_for_debug_bind (sym);
 	  if (tracked_var)
 	    {
 	      gimple note = gimple_build_debug_bind (tracked_var, def, stmt);
 	      /* If stmt ends the bb, insert the debug stmt on the single
 		 non-EH edge from the stmt.  */
 	      if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
 		{
 		  basic_block bb = gsi_bb (gsi);
 		  edge_iterator ei;
 		  edge e, ef = NULL;
Index: gcc/tree-ssa-live.c
===================================================================
--- gcc/tree-ssa-live.c	(revision 216689)
+++ gcc/tree-ssa-live.c	(working copy)
@@ -40,20 +40,21 @@ along with GCC; see the file COPYING3.
 #include "stringpool.h"
 #include "tree-ssanames.h"
 #include "expr.h"
 #include "tree-dfa.h"
 #include "timevar.h"
 #include "dumpfile.h"
 #include "tree-ssa-live.h"
 #include "diagnostic-core.h"
 #include "debug.h"
 #include "flags.h"
+#include "tree-ssa.h"
 
 #ifdef ENABLE_CHECKING
 static void  verify_live_on_entry (tree_live_info_p);
 #endif
 
 
 /* VARMAP maintains a mapping from SSA version number to real variables.
 
    All SSA_NAMES are divided into partitions.  Initially each ssa_name is the
    only member of it's own partition.  Coalescing will attempt to group any
@@ -1086,20 +1087,24 @@ set_var_live_on_entry (tree ssa_name, tr
   if (stmt)
     {
       def_bb = gimple_bb (stmt);
       /* Mark defs in liveout bitmap temporarily.  */
       if (def_bb)
 	bitmap_set_bit (&live->liveout[def_bb->index], p);
     }
   else
     def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
 
+  /* An undefined local variable does not need to be very alive.  */
+  if (ssa_undefined_value_p (ssa_name, false))
+    return;
+
   /* Visit each use of SSA_NAME and if it isn't in the same block as the def,
      add it to the list of live on entry blocks.  */
   FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
     {
       gimple use_stmt = USE_STMT (use);
       basic_block add_block = NULL;
 
       if (gimple_code (use_stmt) == GIMPLE_PHI)
         {
 	  /* Uses in PHI's are considered to be live at exit of the SRC block
@@ -1422,20 +1427,25 @@ verify_live_on_entry (tree_live_info_p l
 			  fprintf (stderr, "\n");
 			}
 		      else
 			fprintf (stderr, " and there is no default def.\n");
 		    }
 		}
 	    }
 	  else
 	    if (d == var)
 	      {
+		/* An undefined local variable does not need to be very
+		   alive.  */
+		if (ssa_undefined_value_p (var, false))
+		  continue;
+
 		/* The only way this var shouldn't be marked live on entry is
 		   if it occurs in a PHI argument of the block.  */
 		size_t z;
 		bool ok = false;
 		gimple_stmt_iterator gsi;
 		for (gsi = gsi_start_phis (e->dest);
 		     !gsi_end_p (gsi) && !ok;
 		     gsi_next (&gsi))
 		  {
 		    gimple phi = gsi_stmt (gsi);
Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c	(revision 216689)
+++ gcc/tree-ssa.c	(working copy)
@@ -1178,24 +1178,25 @@ tree_ssa_useless_type_conversion (tree e
 
 tree
 tree_ssa_strip_useless_type_conversions (tree exp)
 {
   while (tree_ssa_useless_type_conversion (exp))
     exp = TREE_OPERAND (exp, 0);
   return exp;
 }
 
 
-/* Return true if T, an SSA_NAME, has an undefined value.  */
+/* Return true if T, an SSA_NAME, has an undefined value.  PARTIAL is what
+   should be returned if the value is only partially undefined.  */
 
 bool
-ssa_undefined_value_p (tree t)
+ssa_undefined_value_p (tree t, bool partial)
 {
   gimple def_stmt;
   tree var = SSA_NAME_VAR (t);
 
   if (!var)
     ;
   /* Parameters get their initial value from the function entry.  */
   else if (TREE_CODE (var) == PARM_DECL)
     return false;
   /* When returning by reference the return address is actually a hidden
@@ -1205,21 +1206,21 @@ ssa_undefined_value_p (tree t)
   /* Hard register variables get their initial value from the ether.  */
   else if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var))
     return false;
 
   /* The value is undefined iff its definition statement is empty.  */
   def_stmt = SSA_NAME_DEF_STMT (t);
   if (gimple_nop_p (def_stmt))
     return true;
 
   /* Check if the complex was not only partially defined.  */
-  if (is_gimple_assign (def_stmt)
+  if (partial && is_gimple_assign (def_stmt)
       && gimple_assign_rhs_code (def_stmt) == COMPLEX_EXPR)
     {
       tree rhs1, rhs2;
 
       rhs1 = gimple_assign_rhs1 (def_stmt);
       rhs2 = gimple_assign_rhs2 (def_stmt);
       return (TREE_CODE (rhs1) == SSA_NAME && ssa_undefined_value_p (rhs1))
 	     || (TREE_CODE (rhs2) == SSA_NAME && ssa_undefined_value_p (rhs2));
     }
   return false;
@@ -1551,32 +1552,20 @@ execute_update_addresses_taken (void)
 		rhs = gimple_assign_rhs1 (stmt);
 		if (gimple_assign_lhs (stmt) != lhs
 		    && !useless_type_conversion_p (TREE_TYPE (lhs),
 						   TREE_TYPE (rhs)))
 		  rhs = fold_build1 (VIEW_CONVERT_EXPR,
 				     TREE_TYPE (lhs), rhs);
 
 		if (gimple_assign_lhs (stmt) != lhs)
 		  gimple_assign_set_lhs (stmt, lhs);
 
-		/* For var ={v} {CLOBBER}; where var lost
-		   TREE_ADDRESSABLE just remove the stmt.  */
-		if (DECL_P (lhs)
-		    && TREE_CLOBBER_P (rhs)
-		    && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
-		  {
-		    unlink_stmt_vdef (stmt);
-      		    gsi_remove (&gsi, true);
-		    release_defs (stmt);
-		    continue;
-		  }
-
 		if (gimple_assign_rhs1 (stmt) != rhs)
 		  {
 		    gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
 		    gimple_assign_set_rhs_from_tree (&gsi, rhs);
 		  }
 	      }
 
 	    else if (gimple_code (stmt) == GIMPLE_CALL)
 	      {
 		unsigned i;
Index: gcc/tree-ssa.h
===================================================================
--- gcc/tree-ssa.h	(revision 216689)
+++ gcc/tree-ssa.h	(working copy)
@@ -44,21 +44,21 @@ extern tree target_for_debug_bind (tree)
 extern void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
 extern void insert_debug_temps_for_defs (gimple_stmt_iterator *);
 extern void reset_debug_uses (gimple);
 extern void release_defs_bitset (bitmap toremove);
 extern void verify_ssa (bool, bool);
 extern void init_tree_ssa (struct function *);
 extern void delete_tree_ssa (void);
 extern bool tree_ssa_useless_type_conversion (tree);
 extern tree tree_ssa_strip_useless_type_conversions (tree);
 
-extern bool ssa_undefined_value_p (tree);
+extern bool ssa_undefined_value_p (tree, bool = true);
 extern void execute_update_addresses_taken (void);
 
 /* Given an edge_var_map V, return the PHI arg definition.  */
 
 static inline tree
 redirect_edge_var_map_def (edge_var_map *v)
 {
   return v->def;
 }
 

Re: update address taken: don't drop clobbers

[Richard Biener]

On Sat, Oct 25, 2014 at 6:29 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Fri, 24 Oct 2014, Jeff Law wrote:
>
>> I'm starting to agree -- a later message indicated you wanted to drop the
>> unlink_stmt_vdef call and you wanted to avoid gsi_replace, that seems fine.
>> I'll approve once those things are taken care of.
>
>
> The following passed bootstrap+testsuite. I wasn't sure what exactly a
> clobber is guaranteed not to have (no histograms for instance? At least I
> assumed it doesn't throw) so I may have kept some unnecessary calls when I
> inlined gsi_replace. I'd be happy to remove any you feel is useless.
>
> 2014-10-26  Marc Glisse  <marc.glisse@inria.fr>
>
>         PR tree-optimization/60770
> gcc/
>         * tree-into-ssa.c: Include value-prof.h.
>         (maybe_register_def): Replace clobbers with default definitions.
>         * tree-ssa-live.c: Include tree-ssa.h.
>         (set_var_live_on_entry): Do not mark undefined variables as live.
>         (verify_live_on_entry): Do not check undefined variables.
>         * tree-ssa.h (ssa_undefined_value_p): New parameter for the case
>         of partially undefined variables.
>         * tree-ssa.c (ssa_undefined_value_p): Likewise.
>         (execute_update_addresses_taken): Do not drop clobbers.
>
> gcc/testsuite/
>         * gcc.dg/tree-ssa/pr60770-1.c: New file.
>
> --
> Marc Glisse
>
> Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c   (revision 0)
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c   (working copy)
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -Wall" } */
> +
> +int f(int n){
> +  int*p;
> +  {
> +    int yyy=n;
> +    p=&yyy;
> +  }
> +  return *p; /* { dg-warning "yyy" } */
> +}
> Index: gcc/tree-into-ssa.c
> ===================================================================
> --- gcc/tree-into-ssa.c (revision 216689)
> +++ gcc/tree-into-ssa.c (working copy)
> @@ -52,20 +52,21 @@ along with GCC; see the file COPYING3.
>  #include "expr.h"
>  #include "tree-dfa.h"
>  #include "tree-ssa.h"
>  #include "tree-inline.h"
>  #include "tree-pass.h"
>  #include "cfgloop.h"
>  #include "domwalk.h"
>  #include "params.h"
>  #include "diagnostic-core.h"
>  #include "tree-into-ssa.h"
> +#include "value-prof.h"
>
>  #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y))
>
>  /* This file builds the SSA form for a function as described in:
>     R. Cytron, J. Ferrante, B. Rosen, M. Wegman, and K. Zadeck. Efficiently
>     Computing Static Single Assignment Form and the Control Dependence
>     Graph. ACM Transactions on Programming Languages and Systems,
>     13(4):451-490, October 1991.  */
>
>  /* Structure to map a variable VAR to the set of blocks that contain
> @@ -1837,26 +1838,42 @@ maybe_register_def (def_operand_p def_p,
>  {
>    tree def = DEF_FROM_PTR (def_p);
>    tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>
>    /* If DEF is a naked symbol that needs renaming, create a new
>       name for it.  */
>    if (marked_for_renaming (sym))
>      {
>        if (DECL_P (def))
>         {
> -         tree tracked_var;
> +         if (gimple_clobber_p (stmt) && is_gimple_reg (sym))

I think you know that sym is a gimple-reg as the code previously
unconditionally generated an SSA name for it.

> +           {
> +             /* Replace clobber stmts with a default def. This new use of a
> +                default definition may make it look like SSA_NAMEs have
> +                conflicting lifetimes, so we need special code to let them
> +                coalesce properly.  */
> +             /* Hand-inlined version of the following, for safety
> +                gsi_replace (&gsi, gimple_build_nop (), true);  */
> +             gimple nop = gimple_build_nop ();
> +             gimple_set_bb (nop, gsi_bb (gsi));
> +             gimple_set_bb (stmt, NULL);
> +             gimple_remove_stmt_histograms (cfun, stmt);
> +             delink_stmt_imm_use (stmt);
> +             gsi_set_stmt (&gsi, nop);

Is there any reason for this dance?  I'd rather have maybe_register_def
return a bool whether to remove the stmt... passing it down to the
single caller of rewrite_update_stmt which can then gsi_remove the
stmt.

> -         def = make_ssa_name (def, stmt);
> +             def = get_or_create_ssa_default_def (cfun, sym);

I think if 'def' turns out to be a PARM_DECL this does the wrong
thing (well, not technically wrong...  but maybe unexpected).  Not
sure if we ever end up with a PARM = {} clobber though.  Maybe
guard all this with TREE_CODE (def) == VAR_DECL for extra
safety.

Otherwise the patch looks ok.

Thanks for your patience.
Richard.

> +           }
> +         else
> +           def = make_ssa_name (def, stmt);
>           SET_DEF (def_p, def);
>
> -         tracked_var = target_for_debug_bind (sym);
> +         tree tracked_var = target_for_debug_bind (sym);
>           if (tracked_var)
>             {
>               gimple note = gimple_build_debug_bind (tracked_var, def,
> stmt);
>               /* If stmt ends the bb, insert the debug stmt on the single
>                  non-EH edge from the stmt.  */
>               if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
>                 {
>                   basic_block bb = gsi_bb (gsi);
>                   edge_iterator ei;
>                   edge e, ef = NULL;
> Index: gcc/tree-ssa-live.c
> ===================================================================
> --- gcc/tree-ssa-live.c (revision 216689)
> +++ gcc/tree-ssa-live.c (working copy)
> @@ -40,20 +40,21 @@ along with GCC; see the file COPYING3.
>  #include "stringpool.h"
>  #include "tree-ssanames.h"
>  #include "expr.h"
>  #include "tree-dfa.h"
>  #include "timevar.h"
>  #include "dumpfile.h"
>  #include "tree-ssa-live.h"
>  #include "diagnostic-core.h"
>  #include "debug.h"
>  #include "flags.h"
> +#include "tree-ssa.h"
>
>  #ifdef ENABLE_CHECKING
>  static void  verify_live_on_entry (tree_live_info_p);
>  #endif
>
>
>  /* VARMAP maintains a mapping from SSA version number to real variables.
>
>     All SSA_NAMES are divided into partitions.  Initially each ssa_name is
> the
>     only member of it's own partition.  Coalescing will attempt to group any
> @@ -1086,20 +1087,24 @@ set_var_live_on_entry (tree ssa_name, tr
>    if (stmt)
>      {
>        def_bb = gimple_bb (stmt);
>        /* Mark defs in liveout bitmap temporarily.  */
>        if (def_bb)
>         bitmap_set_bit (&live->liveout[def_bb->index], p);
>      }
>    else
>      def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
>
> +  /* An undefined local variable does not need to be very alive.  */
> +  if (ssa_undefined_value_p (ssa_name, false))
> +    return;
> +
>    /* Visit each use of SSA_NAME and if it isn't in the same block as the
> def,
>       add it to the list of live on entry blocks.  */
>    FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
>      {
>        gimple use_stmt = USE_STMT (use);
>        basic_block add_block = NULL;
>
>        if (gimple_code (use_stmt) == GIMPLE_PHI)
>          {
>           /* Uses in PHI's are considered to be live at exit of the SRC
> block
> @@ -1422,20 +1427,25 @@ verify_live_on_entry (tree_live_info_p l
>                           fprintf (stderr, "\n");
>                         }
>                       else
>                         fprintf (stderr, " and there is no default def.\n");
>                     }
>                 }
>             }
>           else
>             if (d == var)
>               {
> +               /* An undefined local variable does not need to be very
> +                  alive.  */
> +               if (ssa_undefined_value_p (var, false))
> +                 continue;
> +
>                 /* The only way this var shouldn't be marked live on entry
> is
>                    if it occurs in a PHI argument of the block.  */
>                 size_t z;
>                 bool ok = false;
>                 gimple_stmt_iterator gsi;
>                 for (gsi = gsi_start_phis (e->dest);
>                      !gsi_end_p (gsi) && !ok;
>                      gsi_next (&gsi))
>                   {
>                     gimple phi = gsi_stmt (gsi);
> Index: gcc/tree-ssa.c
> ===================================================================
> --- gcc/tree-ssa.c      (revision 216689)
> +++ gcc/tree-ssa.c      (working copy)
> @@ -1178,24 +1178,25 @@ tree_ssa_useless_type_conversion (tree e
>
>  tree
>  tree_ssa_strip_useless_type_conversions (tree exp)
>  {
>    while (tree_ssa_useless_type_conversion (exp))
>      exp = TREE_OPERAND (exp, 0);
>    return exp;
>  }
>
>
> -/* Return true if T, an SSA_NAME, has an undefined value.  */
> +/* Return true if T, an SSA_NAME, has an undefined value.  PARTIAL is what
> +   should be returned if the value is only partially undefined.  */
>
>  bool
> -ssa_undefined_value_p (tree t)
> +ssa_undefined_value_p (tree t, bool partial)
>  {
>    gimple def_stmt;
>    tree var = SSA_NAME_VAR (t);
>
>    if (!var)
>      ;
>    /* Parameters get their initial value from the function entry.  */
>    else if (TREE_CODE (var) == PARM_DECL)
>      return false;
>    /* When returning by reference the return address is actually a hidden
> @@ -1205,21 +1206,21 @@ ssa_undefined_value_p (tree t)
>    /* Hard register variables get their initial value from the ether.  */
>    else if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var))
>      return false;
>
>    /* The value is undefined iff its definition statement is empty.  */
>    def_stmt = SSA_NAME_DEF_STMT (t);
>    if (gimple_nop_p (def_stmt))
>      return true;
>
>    /* Check if the complex was not only partially defined.  */
> -  if (is_gimple_assign (def_stmt)
> +  if (partial && is_gimple_assign (def_stmt)
>        && gimple_assign_rhs_code (def_stmt) == COMPLEX_EXPR)
>      {
>        tree rhs1, rhs2;
>
>        rhs1 = gimple_assign_rhs1 (def_stmt);
>        rhs2 = gimple_assign_rhs2 (def_stmt);
>        return (TREE_CODE (rhs1) == SSA_NAME && ssa_undefined_value_p (rhs1))
>              || (TREE_CODE (rhs2) == SSA_NAME && ssa_undefined_value_p
> (rhs2));
>      }
>    return false;
> @@ -1551,32 +1552,20 @@ execute_update_addresses_taken (void)
>                 rhs = gimple_assign_rhs1 (stmt);
>                 if (gimple_assign_lhs (stmt) != lhs
>                     && !useless_type_conversion_p (TREE_TYPE (lhs),
>                                                    TREE_TYPE (rhs)))
>                   rhs = fold_build1 (VIEW_CONVERT_EXPR,
>                                      TREE_TYPE (lhs), rhs);
>
>                 if (gimple_assign_lhs (stmt) != lhs)
>                   gimple_assign_set_lhs (stmt, lhs);
>
> -               /* For var ={v} {CLOBBER}; where var lost
> -                  TREE_ADDRESSABLE just remove the stmt.  */
> -               if (DECL_P (lhs)
> -                   && TREE_CLOBBER_P (rhs)
> -                   && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
> -                 {
> -                   unlink_stmt_vdef (stmt);
> -                   gsi_remove (&gsi, true);
> -                   release_defs (stmt);
> -                   continue;
> -                 }
> -
>                 if (gimple_assign_rhs1 (stmt) != rhs)
>                   {
>                     gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
>                     gimple_assign_set_rhs_from_tree (&gsi, rhs);
>                   }
>               }
>
>             else if (gimple_code (stmt) == GIMPLE_CALL)
>               {
>                 unsigned i;
> Index: gcc/tree-ssa.h
> ===================================================================
> --- gcc/tree-ssa.h      (revision 216689)
> +++ gcc/tree-ssa.h      (working copy)
> @@ -44,21 +44,21 @@ extern tree target_for_debug_bind (tree)
>  extern void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
>  extern void insert_debug_temps_for_defs (gimple_stmt_iterator *);
>  extern void reset_debug_uses (gimple);
>  extern void release_defs_bitset (bitmap toremove);
>  extern void verify_ssa (bool, bool);
>  extern void init_tree_ssa (struct function *);
>  extern void delete_tree_ssa (void);
>  extern bool tree_ssa_useless_type_conversion (tree);
>  extern tree tree_ssa_strip_useless_type_conversions (tree);
>
> -extern bool ssa_undefined_value_p (tree);
> +extern bool ssa_undefined_value_p (tree, bool = true);
>  extern void execute_update_addresses_taken (void);
>
>  /* Given an edge_var_map V, return the PHI arg definition.  */
>
>  static inline tree
>  redirect_edge_var_map_def (edge_var_map *v)
>  {
>    return v->def;
>  }
>
>

Re: update address taken: don't drop clobbers

[Marc Glisse]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7345 bytes --]

On Fri, 31 Oct 2014, Richard Biener wrote:

> On Sat, Oct 25, 2014 at 6:29 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> On Fri, 24 Oct 2014, Jeff Law wrote:
>>
>>> I'm starting to agree -- a later message indicated you wanted to drop the
>>> unlink_stmt_vdef call and you wanted to avoid gsi_replace, that seems fine.
>>> I'll approve once those things are taken care of.
>>
>>
>> The following passed bootstrap+testsuite. I wasn't sure what exactly a
>> clobber is guaranteed not to have (no histograms for instance? At least I
>> assumed it doesn't throw) so I may have kept some unnecessary calls when I
>> inlined gsi_replace. I'd be happy to remove any you feel is useless.
>>
>> 2014-10-26  Marc Glisse  <marc.glisse@inria.fr>
>>
>>         PR tree-optimization/60770
>> gcc/
>>         * tree-into-ssa.c: Include value-prof.h.
>>         (maybe_register_def): Replace clobbers with default definitions.
>>         * tree-ssa-live.c: Include tree-ssa.h.
>>         (set_var_live_on_entry): Do not mark undefined variables as live.
>>         (verify_live_on_entry): Do not check undefined variables.
>>         * tree-ssa.h (ssa_undefined_value_p): New parameter for the case
>>         of partially undefined variables.
>>         * tree-ssa.c (ssa_undefined_value_p): Likewise.
>>         (execute_update_addresses_taken): Do not drop clobbers.
>>
>> gcc/testsuite/
>>         * gcc.dg/tree-ssa/pr60770-1.c: New file.
>>
>> --
>> Marc Glisse
>>
>> Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
>> ===================================================================
>> --- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c   (revision 0)
>> +++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c   (working copy)
>> @@ -0,0 +1,11 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O -Wall" } */
>> +
>> +int f(int n){
>> +  int*p;
>> +  {
>> +    int yyy=n;
>> +    p=&yyy;
>> +  }
>> +  return *p; /* { dg-warning "yyy" } */
>> +}
>> Index: gcc/tree-into-ssa.c
>> ===================================================================
>> --- gcc/tree-into-ssa.c (revision 216689)
>> +++ gcc/tree-into-ssa.c (working copy)
>> @@ -52,20 +52,21 @@ along with GCC; see the file COPYING3.
>>  #include "expr.h"
>>  #include "tree-dfa.h"
>>  #include "tree-ssa.h"
>>  #include "tree-inline.h"
>>  #include "tree-pass.h"
>>  #include "cfgloop.h"
>>  #include "domwalk.h"
>>  #include "params.h"
>>  #include "diagnostic-core.h"
>>  #include "tree-into-ssa.h"
>> +#include "value-prof.h"
>>
>>  #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y))
>>
>>  /* This file builds the SSA form for a function as described in:
>>     R. Cytron, J. Ferrante, B. Rosen, M. Wegman, and K. Zadeck. Efficiently
>>     Computing Static Single Assignment Form and the Control Dependence
>>     Graph. ACM Transactions on Programming Languages and Systems,
>>     13(4):451-490, October 1991.  */
>>
>>  /* Structure to map a variable VAR to the set of blocks that contain
>> @@ -1837,26 +1838,42 @@ maybe_register_def (def_operand_p def_p,
>>  {
>>    tree def = DEF_FROM_PTR (def_p);
>>    tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>
>>    /* If DEF is a naked symbol that needs renaming, create a new
>>       name for it.  */
>>    if (marked_for_renaming (sym))
>>      {
>>        if (DECL_P (def))
>>         {
>> -         tree tracked_var;
>> +         if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>
> I think you know that sym is a gimple-reg as the code previously
> unconditionally generated an SSA name for it.

I checked that in July and it failed:
https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01828.html

>> +           {
>> +             /* Replace clobber stmts with a default def. This new use of a
>> +                default definition may make it look like SSA_NAMEs have
>> +                conflicting lifetimes, so we need special code to let them
>> +                coalesce properly.  */
>> +             /* Hand-inlined version of the following, for safety
>> +                gsi_replace (&gsi, gimple_build_nop (), true);  */
>> +             gimple nop = gimple_build_nop ();
>> +             gimple_set_bb (nop, gsi_bb (gsi));
>> +             gimple_set_bb (stmt, NULL);
>> +             gimple_remove_stmt_histograms (cfun, stmt);
>> +             delink_stmt_imm_use (stmt);
>> +             gsi_set_stmt (&gsi, nop);
>
> Is there any reason for this dance?  I'd rather have maybe_register_def
> return a bool whether to remove the stmt... passing it down to the
> single caller of rewrite_update_stmt which can then gsi_remove the
> stmt.

For more context, my starting point was the code in rewrite_stmt, which
I was trying to port to rewrite_update_stmt (and thus
maybe_register_def):

         if (gimple_clobber_p (stmt)
             && is_gimple_reg (var))
           {
             /* If we rewrite a DECL into SSA form then drop its
                clobber stmts and replace uses with a new default def.  */
             gcc_checking_assert (TREE_CODE (var) == VAR_DECL
                                  && !gimple_vdef (stmt));
             gsi_replace (si, gimple_build_nop (), true);
             register_new_def (get_or_create_ssa_default_def (cfun, var), var);
             break;
           }

I would be happy using the same gsi_replace line, but you said that it
is dangerous because it runs update_stmt (though I don't see what bad
thing may happen when replacing a clobber by a nop), so I tried to do
the same thing as gsi_replace without update_stmt... Though now that I 
re-read https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01656.html it is not 
clear that you are really opposed to the gsi_replace version.

I tried again with gsi_remove, trying to understand why it was failing 
before, and that is because I was calling it in maybe_register_def instead 
of delegating to whoever calls gsi_next.

>> -         def = make_ssa_name (def, stmt);
>> +             def = get_or_create_ssa_default_def (cfun, sym);
>
> I think if 'def' turns out to be a PARM_DECL this does the wrong
> thing (well, not technically wrong...  but maybe unexpected).  Not
> sure if we ever end up with a PARM = {} clobber though.  Maybe
> guard all this with TREE_CODE (def) == VAR_DECL for extra
> safety.

Hmm, you are right. The rewrite_stmt version has

             gcc_checking_assert (TREE_CODE (var) == VAR_DECL && ...

I don't remember exactly why I removed it, maybe because the second part 
of the assertion was failing.

Here is a new version, that passed bootstrap+testsuite on x86_64-linux-gnu.

2014-11-03  Marc Glisse  <marc.glisse@inria.fr>

 	PR tree-optimization/60770
gcc/
 	* tree-into-ssa.c (rewrite_update_stmt): Return whether the
 	statement should be removed.
 	(maybe_register_def): Likewise. Replace clobbers with default
 	definitions.
 	(rewrite_dom_walker::before_dom_children): Remove statement if
 	rewrite_update_stmt says so.
 	* tree-ssa-live.c: Include tree-ssa.h.
 	(set_var_live_on_entry): Do not mark undefined variables as live.
 	(verify_live_on_entry): Do not check undefined variables.
 	* tree-ssa.h (ssa_undefined_value_p): New parameter for the case
 	of partially undefined variables.
 	* tree-ssa.c (ssa_undefined_value_p): Likewise.
 	(execute_update_addresses_taken): Do not drop clobbers.

gcc/testsuite/
 	* gcc.dg/tree-ssa/pr60770-1.c: New file.

-- 
Marc Glisse

[-- Attachment #2: Type: TEXT/PLAIN, Size: 12495 bytes --]

Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c	(revision 0)
+++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c	(working copy)
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-O -Wall" } */
+
+int f(int n){
+  int*p;
+  {
+    int yyy=n;
+    p=&yyy;
+  }
+  return *p; /* { dg-warning "yyy" } */
+}
Index: gcc/tree-into-ssa.c
===================================================================
--- gcc/tree-into-ssa.c	(revision 217007)
+++ gcc/tree-into-ssa.c	(working copy)
@@ -1826,41 +1826,51 @@ maybe_replace_use_in_debug_stmt (use_ope
   if (rdef && rdef != use)
     SET_USE (use_p, rdef);
 
   return rdef != NULL_TREE;
 }
 
 
 /* If the operand pointed to by DEF_P is an SSA name in NEW_SSA_NAMES
    or OLD_SSA_NAMES, or if it is a symbol marked for renaming,
    register it as the current definition for the names replaced by
-   DEF_P.  */
+   DEF_P.  Returns whether the statement should be removed.  */
 
-static inline void
+static inline bool
 maybe_register_def (def_operand_p def_p, gimple stmt,
 		    gimple_stmt_iterator gsi)
 {
   tree def = DEF_FROM_PTR (def_p);
   tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
+  bool to_delete = false;
 
   /* If DEF is a naked symbol that needs renaming, create a new
      name for it.  */
   if (marked_for_renaming (sym))
     {
       if (DECL_P (def))
 	{
-	  tree tracked_var;
-
-	  def = make_ssa_name (def, stmt);
+	  if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
+	    {
+	      gcc_checking_assert (TREE_CODE (sym) == VAR_DECL);
+	      /* Replace clobber stmts with a default def. This new use of a
+		 default definition may make it look like SSA_NAMEs have
+		 conflicting lifetimes, so we need special code to let them
+		 coalesce properly.  */
+	      to_delete = true;
+	      def = get_or_create_ssa_default_def (cfun, sym);
+	    }
+	  else
+	    def = make_ssa_name (def, stmt);
 	  SET_DEF (def_p, def);
 
-	  tracked_var = target_for_debug_bind (sym);
+	  tree tracked_var = target_for_debug_bind (sym);
 	  if (tracked_var)
 	    {
 	      gimple note = gimple_build_debug_bind (tracked_var, def, stmt);
 	      /* If stmt ends the bb, insert the debug stmt on the single
 		 non-EH edge from the stmt.  */
 	      if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
 		{
 		  basic_block bb = gsi_bb (gsi);
 		  edge_iterator ei;
 		  edge e, ef = NULL;
@@ -1904,40 +1914,42 @@ maybe_register_def (def_operand_p def_p,
       /* If DEF is a new name, register it as a new definition
 	 for all the names replaced by DEF.  */
       if (is_new_name (def))
 	register_new_update_set (def, names_replaced_by (def));
 
       /* If DEF is an old name, register DEF as a new
 	 definition for itself.  */
       if (is_old_name (def))
 	register_new_update_single (def, def);
     }
+
+  return to_delete;
 }
 
 
 /* Update every variable used in the statement pointed-to by SI.  The
    statement is assumed to be in SSA form already.  Names in
    OLD_SSA_NAMES used by SI will be updated to their current reaching
    definition.  Names in OLD_SSA_NAMES or NEW_SSA_NAMES defined by SI
    will be registered as a new definition for their corresponding name
-   in OLD_SSA_NAMES.  */
+   in OLD_SSA_NAMES.  Returns whether STMT should be removed.  */
 
-static void
+static bool
 rewrite_update_stmt (gimple stmt, gimple_stmt_iterator gsi)
 {
   use_operand_p use_p;
   def_operand_p def_p;
   ssa_op_iter iter;
 
   /* Only update marked statements.  */
   if (!rewrite_uses_p (stmt) && !register_defs_p (stmt))
-    return;
+    return false;
 
   if (dump_file && (dump_flags & TDF_DETAILS))
     {
       fprintf (dump_file, "Updating SSA information for statement ");
       print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
     }
 
   /* Rewrite USES included in OLD_SSA_NAMES and USES whose underlying
      symbol is marked for renaming.  */
   if (rewrite_uses_p (stmt))
@@ -1974,23 +1986,26 @@ rewrite_update_stmt (gimple stmt, gimple
       else
 	{
 	  FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_ALL_USES)
 	    maybe_replace_use (use_p);
 	}
     }
 
   /* Register definitions of names in NEW_SSA_NAMES and OLD_SSA_NAMES.
      Also register definitions for names whose underlying symbol is
      marked for renaming.  */
+  bool to_delete = false;
   if (register_defs_p (stmt))
     FOR_EACH_SSA_DEF_OPERAND (def_p, stmt, iter, SSA_OP_ALL_DEFS)
-      maybe_register_def (def_p, stmt, gsi);
+      to_delete |= maybe_register_def (def_p, stmt, gsi);
+
+  return to_delete;
 }
 
 
 /* Visit all the successor blocks of BB looking for PHI nodes.  For
    every PHI node found, check if any of its arguments is in
    OLD_SSA_NAMES.  If so, and if the argument has a current reaching
    definition, replace it.  */
 
 static void
 rewrite_update_phi_arguments (basic_block bb)
@@ -2142,22 +2157,25 @@ rewrite_update_dom_walker::before_dom_ch
 	}
 
       if (is_abnormal_phi)
 	SSA_NAME_OCCURS_IN_ABNORMAL_PHI (lhs) = 1;
     }
 
   /* Step 2.  Rewrite every variable used in each statement in the block.  */
   if (bitmap_bit_p (interesting_blocks, bb->index))
     {
       gcc_checking_assert (bitmap_bit_p (blocks_to_update, bb->index));
-      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
-        rewrite_update_stmt (gsi_stmt (gsi), gsi);
+      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
+	if (rewrite_update_stmt (gsi_stmt (gsi), gsi))
+	  gsi_remove (&gsi, true);
+	else
+	  gsi_next (&gsi);
     }
 
   /* Step 3.  Update PHI nodes.  */
   rewrite_update_phi_arguments (bb);
 }
 
 /* Called after visiting block BB.  Unwind BLOCK_DEFS_STACK to restore
    the current reaching definition of every name re-written in BB to
    the original reaching definition before visiting BB.  This
    unwinding must be done in the opposite order to what is done in
Index: gcc/tree-ssa-live.c
===================================================================
--- gcc/tree-ssa-live.c	(revision 217007)
+++ gcc/tree-ssa-live.c	(working copy)
@@ -50,20 +50,21 @@ along with GCC; see the file COPYING3.
 #include "stringpool.h"
 #include "tree-ssanames.h"
 #include "expr.h"
 #include "tree-dfa.h"
 #include "timevar.h"
 #include "dumpfile.h"
 #include "tree-ssa-live.h"
 #include "diagnostic-core.h"
 #include "debug.h"
 #include "flags.h"
+#include "tree-ssa.h"
 
 #ifdef ENABLE_CHECKING
 static void  verify_live_on_entry (tree_live_info_p);
 #endif
 
 
 /* VARMAP maintains a mapping from SSA version number to real variables.
 
    All SSA_NAMES are divided into partitions.  Initially each ssa_name is the
    only member of it's own partition.  Coalescing will attempt to group any
@@ -1096,20 +1097,24 @@ set_var_live_on_entry (tree ssa_name, tr
   if (stmt)
     {
       def_bb = gimple_bb (stmt);
       /* Mark defs in liveout bitmap temporarily.  */
       if (def_bb)
 	bitmap_set_bit (&live->liveout[def_bb->index], p);
     }
   else
     def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
 
+  /* An undefined local variable does not need to be very alive.  */
+  if (ssa_undefined_value_p (ssa_name, false))
+    return;
+
   /* Visit each use of SSA_NAME and if it isn't in the same block as the def,
      add it to the list of live on entry blocks.  */
   FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
     {
       gimple use_stmt = USE_STMT (use);
       basic_block add_block = NULL;
 
       if (gimple_code (use_stmt) == GIMPLE_PHI)
         {
 	  /* Uses in PHI's are considered to be live at exit of the SRC block
@@ -1432,20 +1437,25 @@ verify_live_on_entry (tree_live_info_p l
 			  fprintf (stderr, "\n");
 			}
 		      else
 			fprintf (stderr, " and there is no default def.\n");
 		    }
 		}
 	    }
 	  else
 	    if (d == var)
 	      {
+		/* An undefined local variable does not need to be very
+		   alive.  */
+		if (ssa_undefined_value_p (var, false))
+		  continue;
+
 		/* The only way this var shouldn't be marked live on entry is
 		   if it occurs in a PHI argument of the block.  */
 		size_t z;
 		bool ok = false;
 		gimple_stmt_iterator gsi;
 		for (gsi = gsi_start_phis (e->dest);
 		     !gsi_end_p (gsi) && !ok;
 		     gsi_next (&gsi))
 		  {
 		    gimple phi = gsi_stmt (gsi);
Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c	(revision 217007)
+++ gcc/tree-ssa.c	(working copy)
@@ -1181,24 +1181,25 @@ tree_ssa_useless_type_conversion (tree e
 
 tree
 tree_ssa_strip_useless_type_conversions (tree exp)
 {
   while (tree_ssa_useless_type_conversion (exp))
     exp = TREE_OPERAND (exp, 0);
   return exp;
 }
 
 
-/* Return true if T, an SSA_NAME, has an undefined value.  */
+/* Return true if T, an SSA_NAME, has an undefined value.  PARTIAL is what
+   should be returned if the value is only partially undefined.  */
 
 bool
-ssa_undefined_value_p (tree t)
+ssa_undefined_value_p (tree t, bool partial)
 {
   gimple def_stmt;
   tree var = SSA_NAME_VAR (t);
 
   if (!var)
     ;
   /* Parameters get their initial value from the function entry.  */
   else if (TREE_CODE (var) == PARM_DECL)
     return false;
   /* When returning by reference the return address is actually a hidden
@@ -1208,21 +1209,21 @@ ssa_undefined_value_p (tree t)
   /* Hard register variables get their initial value from the ether.  */
   else if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var))
     return false;
 
   /* The value is undefined iff its definition statement is empty.  */
   def_stmt = SSA_NAME_DEF_STMT (t);
   if (gimple_nop_p (def_stmt))
     return true;
 
   /* Check if the complex was not only partially defined.  */
-  if (is_gimple_assign (def_stmt)
+  if (partial && is_gimple_assign (def_stmt)
       && gimple_assign_rhs_code (def_stmt) == COMPLEX_EXPR)
     {
       tree rhs1, rhs2;
 
       rhs1 = gimple_assign_rhs1 (def_stmt);
       rhs2 = gimple_assign_rhs2 (def_stmt);
       return (TREE_CODE (rhs1) == SSA_NAME && ssa_undefined_value_p (rhs1))
 	     || (TREE_CODE (rhs2) == SSA_NAME && ssa_undefined_value_p (rhs2));
     }
   return false;
@@ -1554,32 +1555,20 @@ execute_update_addresses_taken (void)
 		rhs = gimple_assign_rhs1 (stmt);
 		if (gimple_assign_lhs (stmt) != lhs
 		    && !useless_type_conversion_p (TREE_TYPE (lhs),
 						   TREE_TYPE (rhs)))
 		  rhs = fold_build1 (VIEW_CONVERT_EXPR,
 				     TREE_TYPE (lhs), rhs);
 
 		if (gimple_assign_lhs (stmt) != lhs)
 		  gimple_assign_set_lhs (stmt, lhs);
 
-		/* For var ={v} {CLOBBER}; where var lost
-		   TREE_ADDRESSABLE just remove the stmt.  */
-		if (DECL_P (lhs)
-		    && TREE_CLOBBER_P (rhs)
-		    && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
-		  {
-		    unlink_stmt_vdef (stmt);
-      		    gsi_remove (&gsi, true);
-		    release_defs (stmt);
-		    continue;
-		  }
-
 		if (gimple_assign_rhs1 (stmt) != rhs)
 		  {
 		    gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
 		    gimple_assign_set_rhs_from_tree (&gsi, rhs);
 		  }
 	      }
 
 	    else if (gimple_code (stmt) == GIMPLE_CALL)
 	      {
 		unsigned i;
Index: gcc/tree-ssa.h
===================================================================
--- gcc/tree-ssa.h	(revision 217007)
+++ gcc/tree-ssa.h	(working copy)
@@ -44,21 +44,21 @@ extern tree target_for_debug_bind (tree)
 extern void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
 extern void insert_debug_temps_for_defs (gimple_stmt_iterator *);
 extern void reset_debug_uses (gimple);
 extern void release_defs_bitset (bitmap toremove);
 extern void verify_ssa (bool, bool);
 extern void init_tree_ssa (struct function *);
 extern void delete_tree_ssa (void);
 extern bool tree_ssa_useless_type_conversion (tree);
 extern tree tree_ssa_strip_useless_type_conversions (tree);
 
-extern bool ssa_undefined_value_p (tree);
+extern bool ssa_undefined_value_p (tree, bool = true);
 extern void execute_update_addresses_taken (void);
 
 /* Given an edge_var_map V, return the PHI arg definition.  */
 
 static inline tree
 redirect_edge_var_map_def (edge_var_map *v)
 {
   return v->def;
 }
 

Re: update address taken: don't drop clobbers

[Richard Biener]

On Sun, Nov 2, 2014 at 11:34 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Fri, 31 Oct 2014, Richard Biener wrote:
>
>> On Sat, Oct 25, 2014 at 6:29 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
>>>
>>> On Fri, 24 Oct 2014, Jeff Law wrote:
>>>
>>>> I'm starting to agree -- a later message indicated you wanted to drop
>>>> the
>>>> unlink_stmt_vdef call and you wanted to avoid gsi_replace, that seems
>>>> fine.
>>>> I'll approve once those things are taken care of.
>>>
>>>
>>>
>>> The following passed bootstrap+testsuite. I wasn't sure what exactly a
>>> clobber is guaranteed not to have (no histograms for instance? At least I
>>> assumed it doesn't throw) so I may have kept some unnecessary calls when
>>> I
>>> inlined gsi_replace. I'd be happy to remove any you feel is useless.
>>>
>>> 2014-10-26  Marc Glisse  <marc.glisse@inria.fr>
>>>
>>>         PR tree-optimization/60770
>>> gcc/
>>>         * tree-into-ssa.c: Include value-prof.h.
>>>         (maybe_register_def): Replace clobbers with default definitions.
>>>         * tree-ssa-live.c: Include tree-ssa.h.
>>>         (set_var_live_on_entry): Do not mark undefined variables as live.
>>>         (verify_live_on_entry): Do not check undefined variables.
>>>         * tree-ssa.h (ssa_undefined_value_p): New parameter for the case
>>>         of partially undefined variables.
>>>         * tree-ssa.c (ssa_undefined_value_p): Likewise.
>>>         (execute_update_addresses_taken): Do not drop clobbers.
>>>
>>> gcc/testsuite/
>>>         * gcc.dg/tree-ssa/pr60770-1.c: New file.
>>>
>>> --
>>> Marc Glisse
>>>
>>> Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
>>> ===================================================================
>>> --- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c   (revision 0)
>>> +++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c   (working copy)
>>> @@ -0,0 +1,11 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-options "-O -Wall" } */
>>> +
>>> +int f(int n){
>>> +  int*p;
>>> +  {
>>> +    int yyy=n;
>>> +    p=&yyy;
>>> +  }
>>> +  return *p; /* { dg-warning "yyy" } */
>>> +}
>>> Index: gcc/tree-into-ssa.c
>>> ===================================================================
>>> --- gcc/tree-into-ssa.c (revision 216689)
>>> +++ gcc/tree-into-ssa.c (working copy)
>>> @@ -52,20 +52,21 @@ along with GCC; see the file COPYING3.
>>>  #include "expr.h"
>>>  #include "tree-dfa.h"
>>>  #include "tree-ssa.h"
>>>  #include "tree-inline.h"
>>>  #include "tree-pass.h"
>>>  #include "cfgloop.h"
>>>  #include "domwalk.h"
>>>  #include "params.h"
>>>  #include "diagnostic-core.h"
>>>  #include "tree-into-ssa.h"
>>> +#include "value-prof.h"
>>>
>>>  #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y))
>>>
>>>  /* This file builds the SSA form for a function as described in:
>>>     R. Cytron, J. Ferrante, B. Rosen, M. Wegman, and K. Zadeck.
>>> Efficiently
>>>     Computing Static Single Assignment Form and the Control Dependence
>>>     Graph. ACM Transactions on Programming Languages and Systems,
>>>     13(4):451-490, October 1991.  */
>>>
>>>  /* Structure to map a variable VAR to the set of blocks that contain
>>> @@ -1837,26 +1838,42 @@ maybe_register_def (def_operand_p def_p,
>>>  {
>>>    tree def = DEF_FROM_PTR (def_p);
>>>    tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
>>>
>>>    /* If DEF is a naked symbol that needs renaming, create a new
>>>       name for it.  */
>>>    if (marked_for_renaming (sym))
>>>      {
>>>        if (DECL_P (def))
>>>         {
>>> -         tree tracked_var;
>>> +         if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
>>
>>
>> I think you know that sym is a gimple-reg as the code previously
>> unconditionally generated an SSA name for it.
>
>
> I checked that in July and it failed:
> https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01828.html
>
>>> +           {
>>> +             /* Replace clobber stmts with a default def. This new use
>>> of a
>>> +                default definition may make it look like SSA_NAMEs have
>>> +                conflicting lifetimes, so we need special code to let
>>> them
>>> +                coalesce properly.  */
>>> +             /* Hand-inlined version of the following, for safety
>>> +                gsi_replace (&gsi, gimple_build_nop (), true);  */
>>> +             gimple nop = gimple_build_nop ();
>>> +             gimple_set_bb (nop, gsi_bb (gsi));
>>> +             gimple_set_bb (stmt, NULL);
>>> +             gimple_remove_stmt_histograms (cfun, stmt);
>>> +             delink_stmt_imm_use (stmt);
>>> +             gsi_set_stmt (&gsi, nop);
>>
>>
>> Is there any reason for this dance?  I'd rather have maybe_register_def
>> return a bool whether to remove the stmt... passing it down to the
>> single caller of rewrite_update_stmt which can then gsi_remove the
>> stmt.
>
>
> For more context, my starting point was the code in rewrite_stmt, which
> I was trying to port to rewrite_update_stmt (and thus
> maybe_register_def):
>
>         if (gimple_clobber_p (stmt)
>             && is_gimple_reg (var))
>           {
>             /* If we rewrite a DECL into SSA form then drop its
>                clobber stmts and replace uses with a new default def.  */
>             gcc_checking_assert (TREE_CODE (var) == VAR_DECL
>                                  && !gimple_vdef (stmt));
>             gsi_replace (si, gimple_build_nop (), true);
>             register_new_def (get_or_create_ssa_default_def (cfun, var),
> var);
>             break;
>           }
>
> I would be happy using the same gsi_replace line, but you said that it
> is dangerous because it runs update_stmt (though I don't see what bad
> thing may happen when replacing a clobber by a nop), so I tried to do
> the same thing as gsi_replace without update_stmt... Though now that I
> re-read https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01656.html it is not
> clear that you are really opposed to the gsi_replace version.
>
> I tried again with gsi_remove, trying to understand why it was failing
> before, and that is because I was calling it in maybe_register_def instead
> of delegating to whoever calls gsi_next.
>
>>> -         def = make_ssa_name (def, stmt);
>>> +             def = get_or_create_ssa_default_def (cfun, sym);
>>
>>
>> I think if 'def' turns out to be a PARM_DECL this does the wrong
>> thing (well, not technically wrong...  but maybe unexpected).  Not
>> sure if we ever end up with a PARM = {} clobber though.  Maybe
>> guard all this with TREE_CODE (def) == VAR_DECL for extra
>> safety.
>
>
> Hmm, you are right. The rewrite_stmt version has
>
>             gcc_checking_assert (TREE_CODE (var) == VAR_DECL && ...
>
> I don't remember exactly why I removed it, maybe because the second part of
> the assertion was failing.
>
> Here is a new version, that passed bootstrap+testsuite on x86_64-linux-gnu.

Ok.

Thanks,
Richard.

> 2014-11-03  Marc Glisse  <marc.glisse@inria.fr>
>
>         PR tree-optimization/60770
> gcc/
>         * tree-into-ssa.c (rewrite_update_stmt): Return whether the
>         statement should be removed.
>         (maybe_register_def): Likewise. Replace clobbers with default
>         definitions.
>         (rewrite_dom_walker::before_dom_children): Remove statement if
>         rewrite_update_stmt says so.
>
>         * tree-ssa-live.c: Include tree-ssa.h.
>         (set_var_live_on_entry): Do not mark undefined variables as live.
>         (verify_live_on_entry): Do not check undefined variables.
>         * tree-ssa.h (ssa_undefined_value_p): New parameter for the case
>         of partially undefined variables.
>         * tree-ssa.c (ssa_undefined_value_p): Likewise.
>         (execute_update_addresses_taken): Do not drop clobbers.
>
> gcc/testsuite/
>         * gcc.dg/tree-ssa/pr60770-1.c: New file.
>
> --
> Marc Glisse
>
> Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c   (revision 0)
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c   (working copy)
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O -Wall" } */
> +
> +int f(int n){
> +  int*p;
> +  {
> +    int yyy=n;
> +    p=&yyy;
> +  }
> +  return *p; /* { dg-warning "yyy" } */
> +}
> Index: gcc/tree-into-ssa.c
> ===================================================================
> --- gcc/tree-into-ssa.c (revision 217007)
> +++ gcc/tree-into-ssa.c (working copy)
> @@ -1826,41 +1826,51 @@ maybe_replace_use_in_debug_stmt (use_ope
>    if (rdef && rdef != use)
>      SET_USE (use_p, rdef);
>
>    return rdef != NULL_TREE;
>  }
>
>
>  /* If the operand pointed to by DEF_P is an SSA name in NEW_SSA_NAMES
>     or OLD_SSA_NAMES, or if it is a symbol marked for renaming,
>     register it as the current definition for the names replaced by
> -   DEF_P.  */
> +   DEF_P.  Returns whether the statement should be removed.  */
>
> -static inline void
> +static inline bool
>  maybe_register_def (def_operand_p def_p, gimple stmt,
>                     gimple_stmt_iterator gsi)
>  {
>    tree def = DEF_FROM_PTR (def_p);
>    tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def);
> +  bool to_delete = false;
>
>    /* If DEF is a naked symbol that needs renaming, create a new
>       name for it.  */
>    if (marked_for_renaming (sym))
>      {
>        if (DECL_P (def))
>         {
> -         tree tracked_var;
> -
> -         def = make_ssa_name (def, stmt);
> +         if (gimple_clobber_p (stmt) && is_gimple_reg (sym))
> +           {
> +             gcc_checking_assert (TREE_CODE (sym) == VAR_DECL);
> +             /* Replace clobber stmts with a default def. This new use of a
> +                default definition may make it look like SSA_NAMEs have
> +                conflicting lifetimes, so we need special code to let them
> +                coalesce properly.  */
> +             to_delete = true;
> +             def = get_or_create_ssa_default_def (cfun, sym);
> +           }
> +         else
> +           def = make_ssa_name (def, stmt);
>           SET_DEF (def_p, def);
>
> -         tracked_var = target_for_debug_bind (sym);
> +         tree tracked_var = target_for_debug_bind (sym);
>           if (tracked_var)
>             {
>               gimple note = gimple_build_debug_bind (tracked_var, def,
> stmt);
>               /* If stmt ends the bb, insert the debug stmt on the single
>                  non-EH edge from the stmt.  */
>               if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt))
>                 {
>                   basic_block bb = gsi_bb (gsi);
>                   edge_iterator ei;
>                   edge e, ef = NULL;
> @@ -1904,40 +1914,42 @@ maybe_register_def (def_operand_p def_p,
>        /* If DEF is a new name, register it as a new definition
>          for all the names replaced by DEF.  */
>        if (is_new_name (def))
>         register_new_update_set (def, names_replaced_by (def));
>
>        /* If DEF is an old name, register DEF as a new
>          definition for itself.  */
>        if (is_old_name (def))
>         register_new_update_single (def, def);
>      }
> +
> +  return to_delete;
>  }
>
>
>  /* Update every variable used in the statement pointed-to by SI.  The
>     statement is assumed to be in SSA form already.  Names in
>     OLD_SSA_NAMES used by SI will be updated to their current reaching
>     definition.  Names in OLD_SSA_NAMES or NEW_SSA_NAMES defined by SI
>     will be registered as a new definition for their corresponding name
> -   in OLD_SSA_NAMES.  */
> +   in OLD_SSA_NAMES.  Returns whether STMT should be removed.  */
>
> -static void
> +static bool
>  rewrite_update_stmt (gimple stmt, gimple_stmt_iterator gsi)
>  {
>    use_operand_p use_p;
>    def_operand_p def_p;
>    ssa_op_iter iter;
>
>    /* Only update marked statements.  */
>    if (!rewrite_uses_p (stmt) && !register_defs_p (stmt))
> -    return;
> +    return false;
>
>    if (dump_file && (dump_flags & TDF_DETAILS))
>      {
>        fprintf (dump_file, "Updating SSA information for statement ");
>        print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
>      }
>
>    /* Rewrite USES included in OLD_SSA_NAMES and USES whose underlying
>       symbol is marked for renaming.  */
>    if (rewrite_uses_p (stmt))
> @@ -1974,23 +1986,26 @@ rewrite_update_stmt (gimple stmt, gimple
>        else
>         {
>           FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_ALL_USES)
>             maybe_replace_use (use_p);
>         }
>      }
>
>    /* Register definitions of names in NEW_SSA_NAMES and OLD_SSA_NAMES.
>       Also register definitions for names whose underlying symbol is
>       marked for renaming.  */
> +  bool to_delete = false;
>    if (register_defs_p (stmt))
>      FOR_EACH_SSA_DEF_OPERAND (def_p, stmt, iter, SSA_OP_ALL_DEFS)
> -      maybe_register_def (def_p, stmt, gsi);
> +      to_delete |= maybe_register_def (def_p, stmt, gsi);
> +
> +  return to_delete;
>  }
>
>
>  /* Visit all the successor blocks of BB looking for PHI nodes.  For
>     every PHI node found, check if any of its arguments is in
>     OLD_SSA_NAMES.  If so, and if the argument has a current reaching
>     definition, replace it.  */
>
>  static void
>  rewrite_update_phi_arguments (basic_block bb)
> @@ -2142,22 +2157,25 @@ rewrite_update_dom_walker::before_dom_ch
>         }
>
>        if (is_abnormal_phi)
>         SSA_NAME_OCCURS_IN_ABNORMAL_PHI (lhs) = 1;
>      }
>
>    /* Step 2.  Rewrite every variable used in each statement in the block.
> */
>    if (bitmap_bit_p (interesting_blocks, bb->index))
>      {
>        gcc_checking_assert (bitmap_bit_p (blocks_to_update, bb->index));
> -      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> -        rewrite_update_stmt (gsi_stmt (gsi), gsi);
> +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
> +       if (rewrite_update_stmt (gsi_stmt (gsi), gsi))
> +         gsi_remove (&gsi, true);
> +       else
> +         gsi_next (&gsi);
>      }
>
>    /* Step 3.  Update PHI nodes.  */
>    rewrite_update_phi_arguments (bb);
>  }
>
>  /* Called after visiting block BB.  Unwind BLOCK_DEFS_STACK to restore
>     the current reaching definition of every name re-written in BB to
>     the original reaching definition before visiting BB.  This
>     unwinding must be done in the opposite order to what is done in
> Index: gcc/tree-ssa-live.c
> ===================================================================
> --- gcc/tree-ssa-live.c (revision 217007)
> +++ gcc/tree-ssa-live.c (working copy)
> @@ -50,20 +50,21 @@ along with GCC; see the file COPYING3.
>  #include "stringpool.h"
>  #include "tree-ssanames.h"
>  #include "expr.h"
>  #include "tree-dfa.h"
>  #include "timevar.h"
>  #include "dumpfile.h"
>  #include "tree-ssa-live.h"
>  #include "diagnostic-core.h"
>  #include "debug.h"
>  #include "flags.h"
> +#include "tree-ssa.h"
>
>  #ifdef ENABLE_CHECKING
>  static void  verify_live_on_entry (tree_live_info_p);
>  #endif
>
>
>  /* VARMAP maintains a mapping from SSA version number to real variables.
>
>     All SSA_NAMES are divided into partitions.  Initially each ssa_name is
> the
>     only member of it's own partition.  Coalescing will attempt to group any
> @@ -1096,20 +1097,24 @@ set_var_live_on_entry (tree ssa_name, tr
>    if (stmt)
>      {
>        def_bb = gimple_bb (stmt);
>        /* Mark defs in liveout bitmap temporarily.  */
>        if (def_bb)
>         bitmap_set_bit (&live->liveout[def_bb->index], p);
>      }
>    else
>      def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun);
>
> +  /* An undefined local variable does not need to be very alive.  */
> +  if (ssa_undefined_value_p (ssa_name, false))
> +    return;
> +
>    /* Visit each use of SSA_NAME and if it isn't in the same block as the
> def,
>       add it to the list of live on entry blocks.  */
>    FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name)
>      {
>        gimple use_stmt = USE_STMT (use);
>        basic_block add_block = NULL;
>
>        if (gimple_code (use_stmt) == GIMPLE_PHI)
>          {
>           /* Uses in PHI's are considered to be live at exit of the SRC
> block
> @@ -1432,20 +1437,25 @@ verify_live_on_entry (tree_live_info_p l
>                           fprintf (stderr, "\n");
>                         }
>                       else
>                         fprintf (stderr, " and there is no default def.\n");
>                     }
>                 }
>             }
>           else
>             if (d == var)
>               {
> +               /* An undefined local variable does not need to be very
> +                  alive.  */
> +               if (ssa_undefined_value_p (var, false))
> +                 continue;
> +
>                 /* The only way this var shouldn't be marked live on entry
> is
>                    if it occurs in a PHI argument of the block.  */
>                 size_t z;
>                 bool ok = false;
>                 gimple_stmt_iterator gsi;
>                 for (gsi = gsi_start_phis (e->dest);
>                      !gsi_end_p (gsi) && !ok;
>                      gsi_next (&gsi))
>                   {
>                     gimple phi = gsi_stmt (gsi);
> Index: gcc/tree-ssa.c
> ===================================================================
> --- gcc/tree-ssa.c      (revision 217007)
> +++ gcc/tree-ssa.c      (working copy)
> @@ -1181,24 +1181,25 @@ tree_ssa_useless_type_conversion (tree e
>
>  tree
>  tree_ssa_strip_useless_type_conversions (tree exp)
>  {
>    while (tree_ssa_useless_type_conversion (exp))
>      exp = TREE_OPERAND (exp, 0);
>    return exp;
>  }
>
>
> -/* Return true if T, an SSA_NAME, has an undefined value.  */
> +/* Return true if T, an SSA_NAME, has an undefined value.  PARTIAL is what
> +   should be returned if the value is only partially undefined.  */
>
>  bool
> -ssa_undefined_value_p (tree t)
> +ssa_undefined_value_p (tree t, bool partial)
>  {
>    gimple def_stmt;
>    tree var = SSA_NAME_VAR (t);
>
>    if (!var)
>      ;
>    /* Parameters get their initial value from the function entry.  */
>    else if (TREE_CODE (var) == PARM_DECL)
>      return false;
>    /* When returning by reference the return address is actually a hidden
> @@ -1208,21 +1209,21 @@ ssa_undefined_value_p (tree t)
>    /* Hard register variables get their initial value from the ether.  */
>    else if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var))
>      return false;
>
>    /* The value is undefined iff its definition statement is empty.  */
>    def_stmt = SSA_NAME_DEF_STMT (t);
>    if (gimple_nop_p (def_stmt))
>      return true;
>
>    /* Check if the complex was not only partially defined.  */
> -  if (is_gimple_assign (def_stmt)
> +  if (partial && is_gimple_assign (def_stmt)
>        && gimple_assign_rhs_code (def_stmt) == COMPLEX_EXPR)
>      {
>        tree rhs1, rhs2;
>
>        rhs1 = gimple_assign_rhs1 (def_stmt);
>        rhs2 = gimple_assign_rhs2 (def_stmt);
>        return (TREE_CODE (rhs1) == SSA_NAME && ssa_undefined_value_p (rhs1))
>              || (TREE_CODE (rhs2) == SSA_NAME && ssa_undefined_value_p
> (rhs2));
>      }
>    return false;
> @@ -1554,32 +1555,20 @@ execute_update_addresses_taken (void)
>                 rhs = gimple_assign_rhs1 (stmt);
>                 if (gimple_assign_lhs (stmt) != lhs
>                     && !useless_type_conversion_p (TREE_TYPE (lhs),
>                                                    TREE_TYPE (rhs)))
>                   rhs = fold_build1 (VIEW_CONVERT_EXPR,
>                                      TREE_TYPE (lhs), rhs);
>
>                 if (gimple_assign_lhs (stmt) != lhs)
>                   gimple_assign_set_lhs (stmt, lhs);
>
> -               /* For var ={v} {CLOBBER}; where var lost
> -                  TREE_ADDRESSABLE just remove the stmt.  */
> -               if (DECL_P (lhs)
> -                   && TREE_CLOBBER_P (rhs)
> -                   && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs)))
> -                 {
> -                   unlink_stmt_vdef (stmt);
> -                   gsi_remove (&gsi, true);
> -                   release_defs (stmt);
> -                   continue;
> -                 }
> -
>                 if (gimple_assign_rhs1 (stmt) != rhs)
>                   {
>                     gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
>                     gimple_assign_set_rhs_from_tree (&gsi, rhs);
>                   }
>               }
>
>             else if (gimple_code (stmt) == GIMPLE_CALL)
>               {
>                 unsigned i;
> Index: gcc/tree-ssa.h
> ===================================================================
> --- gcc/tree-ssa.h      (revision 217007)
> +++ gcc/tree-ssa.h      (working copy)
> @@ -44,21 +44,21 @@ extern tree target_for_debug_bind (tree)
>  extern void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree);
>  extern void insert_debug_temps_for_defs (gimple_stmt_iterator *);
>  extern void reset_debug_uses (gimple);
>  extern void release_defs_bitset (bitmap toremove);
>  extern void verify_ssa (bool, bool);
>  extern void init_tree_ssa (struct function *);
>  extern void delete_tree_ssa (void);
>  extern bool tree_ssa_useless_type_conversion (tree);
>  extern tree tree_ssa_strip_useless_type_conversions (tree);
>
> -extern bool ssa_undefined_value_p (tree);
> +extern bool ssa_undefined_value_p (tree, bool = true);
>  extern void execute_update_addresses_taken (void);
>
>  /* Given an edge_var_map V, return the PHI arg definition.  */
>
>  static inline tree
>  redirect_edge_var_map_def (edge_var_map *v)
>  {
>    return v->def;
>  }
>
>

Re: update address taken: don't drop clobbers

[Jeff Law]

On 10/25/14 10:29, Marc Glisse wrote:
> On Fri, 24 Oct 2014, Jeff Law wrote:
>
>> I'm starting to agree -- a later message indicated you wanted to drop
>> the unlink_stmt_vdef call and you wanted to avoid gsi_replace, that
>> seems fine. I'll approve once those things are taken care of.
>
> The following passed bootstrap+testsuite. I wasn't sure what exactly a
> clobber is guaranteed not to have (no histograms for instance? At least
> I assumed it doesn't throw) so I may have kept some unnecessary calls
> when I inlined gsi_replace. I'd be happy to remove any you feel is useless.
>
> 2014-10-26  Marc Glisse  <marc.glisse@inria.fr>
>
>      PR tree-optimization/60770
> gcc/
>      * tree-into-ssa.c: Include value-prof.h.
>      (maybe_register_def): Replace clobbers with default definitions.
>      * tree-ssa-live.c: Include tree-ssa.h.
>      (set_var_live_on_entry): Do not mark undefined variables as live.
>      (verify_live_on_entry): Do not check undefined variables.
>      * tree-ssa.h (ssa_undefined_value_p): New parameter for the case
>      of partially undefined variables.
>      * tree-ssa.c (ssa_undefined_value_p): Likewise.
>      (execute_update_addresses_taken): Do not drop clobbers.
> gcc/testsuite/
>      * gcc.dg/tree-ssa/pr60770-1.c: New file.
A clobber doesn't generate any code, it's really best to think of it as 
a marker.  It doesn't throw, shouldn't have histrograms or anything else 
of importance.

Approved.

Jeff

Re: update address taken: don't drop clobbers

[Jeff Law]

On 10/16/14 05:20, Richard Biener wrote:
>
> It doesn't even know that it has no uses (the variable still needs to be
> written into SSA form).  OTOH it is a missed DSE opportunity before
> update-address-taken?
Perhaps it's a missed DSE prior to update-address-taken, but I suspect 
there aren't many of those in general...  It wouldn't be hard to shove 
in a DSE pass to get some instrumentation across a wider range of code.

>
> As of premature optimization - into-SSA could notice it created SSA
> names with no uses and trigger a fast DCE.
Yea, I'd been pondering that as well.  I'm curious how often we have 
trivially dead code after into-ssa.  If it's most of the time, then 
maybe we just schedule the DCE right after and not even bother to check 
if we've got SSA_NAMEs without any uses.

>
> Btw, I wonder what this odd folding of variadic builtins is about, and why
> it is not done in the stdarg pass (and only there), which would be earlier.
No clue.  I've tried pretty hard not to think about the lowering of 
variadic builtins.  Perhaps the author of that code was trying to keep 
things simple and let the optimizers to the optimization? :-)

jeff

Re: update address taken: don't drop clobbers

[Marc Glisse]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2699 bytes --]

On Thu, 10 Jul 2014, Richard Biener wrote:

> On Sun, Jun 29, 2014 at 12:33 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
>>
>> we currently drop clobbers on variables whose address is not taken anymore.
>> However, rewrite_stmt has code to replace them with an SSA_NAME with a
>> default definition (an uninitialized variable), and I believe
>> rewrite_update_stmt should do the same. This allows us to warn sometimes
>> (see testcase), but during the debugging I also noticed several places where
>> it allowed CCP to simplify further PHIs, so this is also an optimization.
>>
>> In an earlier version of the patch, I was using
>> get_or_create_ssa_default_def (cfun, sym);
>> (I was reusing the same variable). This passed bootstrap+testsuite on all
>> languages except for ada. Indeed, the compiler wanted to coalesce several
>> SSA_NAMEs, including those new ones, in out-of-ssa, but couldn't. There are
>> abnormal PHIs involved. Maybe it shouldn't have insisted on coalescing an
>> undefined ssa_name, maybe something should have prevented us from reaching
>> such a situation, but creating a new variable was the simplest workaround.
>
> Hmm.  We indeed notice "late" that the new names are used in abnormal
> PHIs.  Note that usually rewriting a symbol into SSA form does not
> create overlapping life-ranges - but of course you are possibly introducing
> those by the new use of the default definitions.
>
> Apart from the out-of-SSA patch you proposed elsewhere a possibility
> is to simply never mark undefined SSA names as
> SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
> as must-coalesce).

For "not mark those as must-coalesce", replacing the liveness patch with 
the attached patch also passed the testsuite: I skip undefined variables 
when handling must-coalesce, and let the regular coalescing code handle 
them. I am not sure what happens during expansion though, and bootstrap 
only hits this issue a couple times in ada so it doesn't prove much.

This patch doesn't conflict with the liveness patch, they are rather 
complementary. I didn't test but I am quite confident that having both 
patches would also pass bootstrap+testsuite.

Of course that all becomes unnecessary if we use default definitions of 
new variables instead of always the same old variable, but I can 
understand not wanting all those new artificial variables.

I would be ok with the patch at 
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html
(minus the line with unlink_stmt_vdef, which is indeed unnecessary)
(I looked at what gsi_replace does when replacing a clobber by a nop, and 
it seems harmless, but I can manually inline the non-dead parts of it if 
you want)

-- 
Marc Glisse

[-- Attachment #2: Type: TEXT/PLAIN, Size: 3512 bytes --]

Index: tree-ssa-coalesce.c
===================================================================
--- tree-ssa-coalesce.c	(revision 216415)
+++ tree-ssa-coalesce.c	(working copy)
@@ -36,20 +36,21 @@ along with GCC; see the file COPYING3.
 #include "gimple.h"
 #include "gimple-iterator.h"
 #include "gimple-ssa.h"
 #include "tree-phinodes.h"
 #include "ssa-iterators.h"
 #include "stringpool.h"
 #include "tree-ssanames.h"
 #include "tree-ssa-live.h"
 #include "tree-ssa-coalesce.h"
 #include "diagnostic-core.h"
+#include "tree-ssa.h"
 
 
 /* This set of routines implements a coalesce_list.  This is an object which
    is used to track pairs of ssa_names which are desirable to coalesce
    together to avoid copies.  Costs are associated with each pair, and when
    all desired information has been collected, the object can be used to
    order the pairs for processing.  */
 
 /* This structure defines a pair entry.  */
 
@@ -962,20 +963,22 @@ create_outofssa_var_map (coalesce_list_p
 		  saw_copy = true;
 		  bitmap_set_bit (used_in_copy, SSA_NAME_VERSION (arg));
 		  if ((e->flags & EDGE_ABNORMAL) == 0)
 		    {
 		      int cost = coalesce_cost_edge (e);
 		      if (cost == 1 && has_single_use (arg))
 			add_cost_one_coalesce (cl, ver, SSA_NAME_VERSION (arg));
 		      else
 			add_coalesce (cl, ver, SSA_NAME_VERSION (arg), cost);
 		    }
+		  else if (ssa_undefined_value_p (arg, false))
+		    add_coalesce (cl, ver, SSA_NAME_VERSION (arg), MUST_COALESCE_COST - 1);
 		}
 	    }
 	  if (saw_copy)
 	    bitmap_set_bit (used_in_copy, ver);
 	}
 
       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
         {
 	  stmt = gsi_stmt (gsi);
 
@@ -1189,20 +1192,22 @@ coalesce_partitions (var_map map, ssa_co
       FOR_EACH_EDGE (e, ei, bb->preds)
 	if (e->flags & EDGE_ABNORMAL)
 	  {
 	    gimple_stmt_iterator gsi;
 	    for (gsi = gsi_start_phis (bb); !gsi_end_p (gsi);
 		 gsi_next (&gsi))
 	      {
 		gimple phi = gsi_stmt (gsi);
 		tree res = PHI_RESULT (phi);
 	        tree arg = PHI_ARG_DEF (phi, e->dest_idx);
+		if (ssa_undefined_value_p (arg, false))
+		  continue;
 		int v1 = SSA_NAME_VERSION (res);
 		int v2 = SSA_NAME_VERSION (arg);
 
 		if (debug)
 		  fprintf (debug, "Abnormal coalesce: ");
 
 		if (!attempt_coalesce (map, graph, v1, v2, debug))
 		  fail_abnormal_edge_coalesce (v1, v2);
 	      }
 	  }
@@ -1287,21 +1292,22 @@ coalesce_ssa_name (void)
 		{
 		  /* If the variable is a PARM_DECL or a RESULT_DECL, we
 		     _require_ that all the names originating from it be
 		     coalesced, because there must be a single partition
 		     containing all the names so that it can be assigned
 		     the canonical RTL location of the DECL safely.
 		     If in_lto_p, a function could have been compiled
 		     originally with optimizations and only the link
 		     performed at -O0, so we can't actually require it.  */
 		  const int cost
-		    = (TREE_CODE (SSA_NAME_VAR (a)) == VAR_DECL || in_lto_p)
+		    = (TREE_CODE (SSA_NAME_VAR (a)) == VAR_DECL || in_lto_p
+		       || ssa_undefined_value_p (a, false))
 		      ? MUST_COALESCE_COST - 1 : MUST_COALESCE_COST;
 		  add_coalesce (cl, SSA_NAME_VERSION (a),
 				SSA_NAME_VERSION (*slot), cost);
 		  bitmap_set_bit (used_in_copies, SSA_NAME_VERSION (a));
 		  bitmap_set_bit (used_in_copies, SSA_NAME_VERSION (*slot));
 		}
 	    }
 	}
     }
   if (dump_file && (dump_flags & TDF_DETAILS))

Re: update address taken: don't drop clobbers

[Jeff Law]

On 10/18/14 15:59, Marc Glisse wrote:
> On Thu, 10 Jul 2014, Richard Biener wrote:
>
>> On Sun, Jun 29, 2014 at 12:33 AM, Marc Glisse <marc.glisse@inria.fr>
>> wrote:
>>>
>>> we currently drop clobbers on variables whose address is not taken
>>> anymore.
>>> However, rewrite_stmt has code to replace them with an SSA_NAME with a
>>> default definition (an uninitialized variable), and I believe
>>> rewrite_update_stmt should do the same. This allows us to warn sometimes
>>> (see testcase), but during the debugging I also noticed several
>>> places where
>>> it allowed CCP to simplify further PHIs, so this is also an
>>> optimization.
>>>
>>> In an earlier version of the patch, I was using
>>> get_or_create_ssa_default_def (cfun, sym);
>>> (I was reusing the same variable). This passed bootstrap+testsuite on
>>> all
>>> languages except for ada. Indeed, the compiler wanted to coalesce
>>> several
>>> SSA_NAMEs, including those new ones, in out-of-ssa, but couldn't.
>>> There are
>>> abnormal PHIs involved. Maybe it shouldn't have insisted on
>>> coalescing an
>>> undefined ssa_name, maybe something should have prevented us from
>>> reaching
>>> such a situation, but creating a new variable was the simplest
>>> workaround.
>>
>> Hmm.  We indeed notice "late" that the new names are used in abnormal
>> PHIs.  Note that usually rewriting a symbol into SSA form does not
>> create overlapping life-ranges - but of course you are possibly
>> introducing
>> those by the new use of the default definitions.
>>
>> Apart from the out-of-SSA patch you proposed elsewhere a possibility
>> is to simply never mark undefined SSA names as
>> SSA_NAME_OCCURS_IN_ABNORMAL_PHI ... (or not mark those
>> as must-coalesce).
>
> For "not mark those as must-coalesce", replacing the liveness patch with
> the attached patch also passed the testsuite: I skip undefined variables
> when handling must-coalesce, and let the regular coalescing code handle
> them. I am not sure what happens during expansion though, and bootstrap
> only hits this issue a couple times in ada so it doesn't prove much.
>
> This patch doesn't conflict with the liveness patch, they are rather
> complementary. I didn't test but I am quite confident that having both
> patches would also pass bootstrap+testsuite.
>
> Of course that all becomes unnecessary if we use default definitions of
> new variables instead of always the same old variable, but I can
> understand not wanting all those new artificial variables.
>
> I would be ok with the patch at
> https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01787.html
> (minus the line with unlink_stmt_vdef, which is indeed unnecessary)
> (I looked at what gsi_replace does when replacing a clobber by a nop,
> and it seems harmless, but I can manually inline the non-dead parts of
> it if you want)
So I'm still trying to get comfortable with this patch.  I guess my 
concerns about having one of the undefined value SSA_NAMEs appearing in 
two conflicting coalesce lists are alleviated by the twiddle to 
coalesce_partitions where we essentially ignore them.

So in the end, they don't end up a part of any partition?  What happens 
when we expand them?  I guess they get a new pseudo since they're a 
distinct partition?  If we had a sensible story for expansion, then I 
could probably get on board with this patch.

And I'm still going to look at the other as well -- as you mention, 
they're independent.

jeff

SRA: don't drop clobbers

[Marc Glisse]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 821 bytes --]

Hello,

now that the update_address_taken patch is in, let me re-post the SRA 
follow-up. With this patch, testcase pr60517.C (attached) has a use of an 
undefined variable at the time of the uninit pass. Sadly, while this 
warned with earlier versions of the other patch (when I was inserting 
default definitions of new variables), it does not anymore because we have 
TREE_NO_WARNING all over the place. Still, I believe this is a step in the 
right direction, and it passed bootstrap+testsuite (minus the new 
testcase). Would it be ok to commit the tree-sra.c change? Then I could 
close PR60770 (the warning itself is PR60517).

2014-11-03  Marc Glisse  <marc.glisse@inria.fr>

 	PR tree-optimization/60770
 	* tree-sra.c (clobber_subtree): New function.
 	(sra_modify_constructor_assign): Call it.

-- 
Marc Glisse

[-- Attachment #2: Type: TEXT/PLAIN, Size: 4108 bytes --]

Index: gcc/testsuite/g++.dg/tree-ssa/pr60517.C
===================================================================
--- gcc/testsuite/g++.dg/tree-ssa/pr60517.C	(revision 0)
+++ gcc/testsuite/g++.dg/tree-ssa/pr60517.C	(working copy)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O -Wall" } */
+
+struct B {
+    double x[2];
+};
+struct A {
+    B b;
+    B getB() { return b; }
+};
+
+double foo(A a) {
+    double * x = &(a.getB().x[0]);
+    return x[0]; /* { dg-warning "" } */
+}
Index: gcc/tree-sra.c
===================================================================
--- gcc/tree-sra.c	(revision 217035)
+++ gcc/tree-sra.c	(working copy)
@@ -2716,20 +2716,51 @@ init_subtree_with_zero (struct access *a
       if (insert_after)
 	gsi_insert_after (gsi, ds, GSI_NEW_STMT);
       else
 	gsi_insert_before (gsi, ds, GSI_SAME_STMT);
     }
 
   for (child = access->first_child; child; child = child->next_sibling)
     init_subtree_with_zero (child, gsi, insert_after, loc);
 }
 
+/* Clobber all scalar replacements in an access subtree.  ACCESS is the the
+   root of the subtree to be processed.  GSI is the statement iterator used
+   for inserting statements which are added after the current statement if
+   INSERT_AFTER is true or before it otherwise.  */
+
+static void
+clobber_subtree (struct access *access, gimple_stmt_iterator *gsi,
+		bool insert_after, location_t loc)
+
+{
+  struct access *child;
+
+  if (access->grp_to_be_replaced)
+    {
+      tree rep = get_access_replacement (access);
+      tree clobber = build_constructor (access->type, NULL);
+      TREE_THIS_VOLATILE (clobber) = 1;
+      gimple stmt = gimple_build_assign (rep, clobber);
+
+      if (insert_after)
+	gsi_insert_after (gsi, stmt, GSI_NEW_STMT);
+      else
+	gsi_insert_before (gsi, stmt, GSI_SAME_STMT);
+      update_stmt (stmt);
+      gimple_set_location (stmt, loc);
+    }
+
+  for (child = access->first_child; child; child = child->next_sibling)
+    clobber_subtree (child, gsi, insert_after, loc);
+}
+
 /* Search for an access representative for the given expression EXPR and
    return it or NULL if it cannot be found.  */
 
 static struct access *
 get_access_for_expr (tree expr)
 {
   HOST_WIDE_INT offset, size, max_size;
   tree base;
 
   /* FIXME: This should not be necessary but Ada produces V_C_Es with a type of
@@ -3028,43 +3059,41 @@ enum assignment_mod_result { SRA_AM_NONE
 			     SRA_AM_REMOVED };  /* stmt eliminated */
 
 /* Modify assignments with a CONSTRUCTOR on their RHS.  STMT contains a pointer
    to the assignment and GSI is the statement iterator pointing at it.  Returns
    the same values as sra_modify_assign.  */
 
 static enum assignment_mod_result
 sra_modify_constructor_assign (gimple stmt, gimple_stmt_iterator *gsi)
 {
   tree lhs = gimple_assign_lhs (stmt);
-  struct access *acc;
-  location_t loc;
-
-  acc = get_access_for_expr (lhs);
+  struct access *acc = get_access_for_expr (lhs);
   if (!acc)
     return SRA_AM_NONE;
+  location_t loc = gimple_location (stmt);
 
   if (gimple_clobber_p (stmt))
     {
-      /* Remove clobbers of fully scalarized variables, otherwise
-	 do nothing.  */
+      /* Clobber the replacement variable.  */
+      clobber_subtree (acc, gsi, !acc->grp_covered, loc);
+      /* Remove clobbers of fully scalarized variables, they are dead.  */
       if (acc->grp_covered)
 	{
 	  unlink_stmt_vdef (stmt);
 	  gsi_remove (gsi, true);
 	  release_defs (stmt);
 	  return SRA_AM_REMOVED;
 	}
       else
-	return SRA_AM_NONE;
+	return SRA_AM_MODIFIED;
     }
 
-  loc = gimple_location (stmt);
   if (vec_safe_length (CONSTRUCTOR_ELTS (gimple_assign_rhs1 (stmt))) > 0)
     {
       /* I have never seen this code path trigger but if it can happen the
 	 following should handle it gracefully.  */
       if (access_has_children_p (acc))
 	generate_subtree_copies (acc->first_child, lhs, acc->offset, 0, 0, gsi,
 				 true, true, loc);
       return SRA_AM_MODIFIED;
     }
 

Re: SRA: don't drop clobbers

[Martin Jambor]

Hi,

On Mon, Nov 03, 2014 at 01:59:24PM +0100, Marc Glisse wrote:
> Hello,
> 
> now that the update_address_taken patch is in, let me re-post the
> SRA follow-up. With this patch, testcase pr60517.C (attached) has a
> use of an undefined variable at the time of the uninit pass. Sadly,
> while this warned with earlier versions of the other patch (when I
> was inserting default definitions of new variables), it does not
> anymore because we have TREE_NO_WARNING all over the place. Still, I
> believe this is a step in the right direction, and it passed
> bootstrap+testsuite (minus the new testcase).

I am not sure what you mean by "minus the new testcase" but, with the
gimple checking compiler at least, the testcase produces an ICE:

  pr60517.C: In function ‘double foo(A)’:
  pr60517.C:15:1: error: non-decl/MEM_REF LHS in clobber statement
   }
   ^
  SR.1_8
  SR.1_8 ={v} {CLOBBER};
  pr60517.C:15:1: internal compiler error: verify_gimple failed
  0xc7810d verify_gimple_in_cfg(function*, bool)
          /home/mjambor/gcc/mine/src/gcc/tree-cfg.c:5039
  0xb8256f execute_function_todo
          /home/mjambor/gcc/mine/src/gcc/passes.c:1758
  0xb83183 execute_todo
          /home/mjambor/gcc/mine/src/gcc/passes.c:1815

The problem is exactly what the verifier complains about, a clobber of
an SSA_NAME which I agree with the verifier is a really bad idea.

If you want SRA to produce information that at a given point a scalar
replacement (which will become an SSA_NAME) does not have any valid
data, the way to represent that is to load it with a value from a
default-definition SSA name.  Something along the lines of
get_repl_default_def_ssa_name which unfortunately you cannot directly
use without confusing the SSA renamer.

The easiest way, at least to test a proof of concept, is probably to
create another declaration of the same type (reusing bits from
create_access_replacement, especially setting the same
DECL_DEBUG_EXPR), get its default-definition using
get_or_create_ssa_default_def and then assign that to the replacement
instead of trying to clobber it.

(A more correct approach would be to design a way of telling the
renamer that at this point the scalar becomes undefined and that it
should start using default-defs until the next definition appears but
that is likely to be difficult, although that is just my guess, it
might be easy.)

> Would it be ok to commit the tree-sra.c change?

I don't think so, sorry about the bad news.

Martin

Re: SRA: don't drop clobbers

[Marc Glisse]

On Mon, 3 Nov 2014, Martin Jambor wrote:

> On Mon, Nov 03, 2014 at 01:59:24PM +0100, Marc Glisse wrote:
>> Hello,
>>
>> now that the update_address_taken patch is in, let me re-post the
>> SRA follow-up. With this patch, testcase pr60517.C (attached) has a
>> use of an undefined variable at the time of the uninit pass. Sadly,
>> while this warned with earlier versions of the other patch (when I
>> was inserting default definitions of new variables), it does not
>> anymore because we have TREE_NO_WARNING all over the place. Still, I
>> believe this is a step in the right direction, and it passed
>> bootstrap+testsuite (minus the new testcase).
>
> I am not sure what you mean by "minus the new testcase" but, with the
> gimple checking compiler at least, the testcase produces an ICE:
>
>  pr60517.C: In function ‘double foo(A)’:
>  pr60517.C:15:1: error: non-decl/MEM_REF LHS in clobber statement
>   }
>   ^
>  SR.1_8
>  SR.1_8 ={v} {CLOBBER};
>  pr60517.C:15:1: internal compiler error: verify_gimple failed
>  0xc7810d verify_gimple_in_cfg(function*, bool)
>          /home/mjambor/gcc/mine/src/gcc/tree-cfg.c:5039
>  0xb8256f execute_function_todo
>          /home/mjambor/gcc/mine/src/gcc/passes.c:1758
>  0xb83183 execute_todo
>          /home/mjambor/gcc/mine/src/gcc/passes.c:1815
>
> The problem is exactly what the verifier complains about, a clobber of
> an SSA_NAME which I agree with the verifier is a really bad idea.
>
> If you want SRA to produce information that at a given point a scalar
> replacement (which will become an SSA_NAME) does not have any valid
> data, the way to represent that is to load it with a value from a
> default-definition SSA name.  Something along the lines of
> get_repl_default_def_ssa_name which unfortunately you cannot directly
> use without confusing the SSA renamer.
>
> The easiest way, at least to test a proof of concept, is probably to
> create another declaration of the same type (reusing bits from
> create_access_replacement, especially setting the same
> DECL_DEBUG_EXPR), get its default-definition using
> get_or_create_ssa_default_def and then assign that to the replacement
> instead of trying to clobber it.
>
> (A more correct approach would be to design a way of telling the
> renamer that at this point the scalar becomes undefined and that it
> should start using default-defs until the next definition appears but
> that is likely to be difficult, although that is just my guess, it
> might be easy.)
>
>> Would it be ok to commit the tree-sra.c change?
>
> I don't think so, sorry about the bad news.

Thanks for your help. I am quite confused. I am not seeing the failure you 
mention, but since I am building from trunk with no specific option I 
should be getting gimple checking.

The way I expect things to work:
1) we have a clobber for a variable V
2) SRA creates a variable SR and replaces uses of V
3) (new) next to V's clobber, we insert a clobber for SR
4) update_address_taken notices that SR does not have its address taken, 
and thus replaces it by SSA_NAMEs, and after the clobber by an undefined 
variable (yesterday it would just have removed the clobber).

What you describe in a parenthesis about the renamer is pretty much what I 
think rewrite_update_stmt does, with the clobber as the hint.

Do you have further comments, based on these explanations, that could help 
me pinpoint what is going wrong?

-- 
Marc Glisse

Re: SRA: don't drop clobbers

[Martin Jambor]

Hi,

On Mon, Nov 03, 2014 at 05:17:22PM +0100, Marc Glisse wrote:
> On Mon, 3 Nov 2014, Martin Jambor wrote:
> >On Mon, Nov 03, 2014 at 01:59:24PM +0100, Marc Glisse wrote:
> >>
> >>now that the update_address_taken patch is in, let me re-post the
> >>SRA follow-up. With this patch, testcase pr60517.C (attached) has a
> >>use of an undefined variable at the time of the uninit pass. Sadly,
> >>while this warned with earlier versions of the other patch (when I
> >>was inserting default definitions of new variables), it does not
> >>anymore because we have TREE_NO_WARNING all over the place. Still, I
> >>believe this is a step in the right direction, and it passed
> >>bootstrap+testsuite (minus the new testcase).
> >
> >I am not sure what you mean by "minus the new testcase" but, with the
> >gimple checking compiler at least, the testcase produces an ICE:
> >
> > pr60517.C: In function ‘double foo(A)’:
> > pr60517.C:15:1: error: non-decl/MEM_REF LHS in clobber statement
> >  }
> >  ^
> > SR.1_8
> > SR.1_8 ={v} {CLOBBER};
> > pr60517.C:15:1: internal compiler error: verify_gimple failed
> > 0xc7810d verify_gimple_in_cfg(function*, bool)
> >         /home/mjambor/gcc/mine/src/gcc/tree-cfg.c:5039
> > 0xb8256f execute_function_todo
> >         /home/mjambor/gcc/mine/src/gcc/passes.c:1758
> > 0xb83183 execute_todo
> >         /home/mjambor/gcc/mine/src/gcc/passes.c:1815
> >
> >The problem is exactly what the verifier complains about, a clobber of
> >an SSA_NAME which I agree with the verifier is a really bad idea.
> >
> >If you want SRA to produce information that at a given point a scalar
> >replacement (which will become an SSA_NAME) does not have any valid
> >data, the way to represent that is to load it with a value from a
> >default-definition SSA name.  Something along the lines of
> >get_repl_default_def_ssa_name which unfortunately you cannot directly
> >use without confusing the SSA renamer.
> >
> >The easiest way, at least to test a proof of concept, is probably to
> >create another declaration of the same type (reusing bits from
> >create_access_replacement, especially setting the same
> >DECL_DEBUG_EXPR), get its default-definition using
> >get_or_create_ssa_default_def and then assign that to the replacement
> >instead of trying to clobber it.
> >
> >(A more correct approach would be to design a way of telling the
> >renamer that at this point the scalar becomes undefined and that it
> >should start using default-defs until the next definition appears but
> >that is likely to be difficult, although that is just my guess, it
> >might be easy.)
> >
> >>Would it be ok to commit the tree-sra.c change?
> >
> >I don't think so, sorry about the bad news.
> 
> Thanks for your help. I am quite confused. I am not seeing the
> failure you mention, but since I am building from trunk with no
> specific option I should be getting gimple checking.
> 
> The way I expect things to work:
> 1) we have a clobber for a variable V
> 2) SRA creates a variable SR and replaces uses of V
> 3) (new) next to V's clobber, we insert a clobber for SR
> 4) update_address_taken notices that SR does not have its address
> taken, and thus replaces it by SSA_NAMEs, and after the clobber by
> an undefined variable (yesterday it would just have removed the
> clobber).

update_address_taken?  The conversion to SSA form is done by means of
returning TODO_update_ssa from the (early) SRA pass (which is what I
mean by the renamer).

> 
> What you describe in a parenthesis about the renamer is pretty much
> what I think rewrite_update_stmt does, with the clobber as the hint.
> 
> Do you have further comments, based on these explanations, that
> could help me pinpoint what is going wrong?

I just applied your patch on top of trunk revision 217032 on my
x86_64-linux (openSUSE 13.1) desktop, configured with

/home/mjambor/gcc/mine/src/configure --prefix=/home/mjambor/gcc/mine/inst --enable-languages=c,c++ --enable-checking=yes --disable-bootstrap --with-plugin-ld=/home/mjambor/binutils/obj/gold/ld-new --disable-libsanitizer --disable-multilib

built with simple make -j8, 

and ran the testcase with make -k check RUNTESTFLAGS="dg.exp=pr60517.C"

And the error popped out in gcc/testsuite/g++/g++.log.  I really can't
think of reason why you don't see it.

But I do and little wonder, really, given the second condition in
verify_gimple_assign_single, which is there for more than a year.
Looking at the dumped statement it seems that the LHS is already an
SSA_NAME.  Moreover, when I dumped the erroneous body in gdb, I have
not seen any default definitions anywhere.  So if your intention was
to teach the renamer to turn this into default definitions, it does
not work and update_ssa did not get the hint.

Martin

Re: SRA: don't drop clobbers

[Marc Glisse]

On Mon, 3 Nov 2014, Martin Jambor wrote:

> I just applied your patch on top of trunk revision 217032 on my

Ah, that explains it, thanks. This patch is a follow-up to r217034. Still, 
I didn't expect the ICE you are seeing by applying this patch to older 
trunk, I'll try to reproduce that.

-- 
Marc Glisse

Re: SRA: don't drop clobbers

[Marc Glisse]

On Mon, 3 Nov 2014, Marc Glisse wrote:

> On Mon, 3 Nov 2014, Martin Jambor wrote:
>
>> I just applied your patch on top of trunk revision 217032 on my
>
> Ah, that explains it, thanks. This patch is a follow-up to r217034. Still, I 
> didn't expect the ICE you are seeing by applying this patch to older trunk, 
> I'll try to reproduce that.

It is TODO_update_address_taken that used to remove clobbers, and as you 
said ESRA goes straight to TODO_update_ssa, which explains why the 
clobbers caused trouble. In any case, after r217034, update_ssa should 
handle clobbers much better. Could you take an other look based on a more 
recent trunk, please?

-- 
Marc Glisse

Re: SRA: don't drop clobbers

[Martin Jambor]

Hi,

On Mon, Nov 03, 2014 at 10:46:49PM +0100, Marc Glisse wrote:
> On Mon, 3 Nov 2014, Marc Glisse wrote:
> 
> >On Mon, 3 Nov 2014, Martin Jambor wrote:
> >
> >>I just applied your patch on top of trunk revision 217032 on my
> >
> >Ah, that explains it, thanks. This patch is a follow-up to
> >r217034. Still, I didn't expect the ICE you are seeing by applying
> >this patch to older trunk, I'll try to reproduce that.
> 
> It is TODO_update_address_taken that used to remove clobbers, and as
> you said ESRA goes straight to TODO_update_ssa, which explains why
> the clobbers caused trouble. In any case, after r217034, update_ssa
> should handle clobbers much better. Could you take an other look
> based on a more recent trunk, please?
> 

Sorry for the delay.  Anyway, on the current trunk (i.e. Tuesday
checkout) the patch works as expected, there are assignments from
default definitions now and even though we do not warn as we should,
the patch improves the generated code.  The function foo from the
testcase is optimized to "return SR.1_2(D);" as soon as release_ssa
now, whereas unpatched trunk leaves an undefined load even in the
optimized dump.

Thus, I like the patch and given that you posted it well before stage1
end, I'd like to see it committed.  Richi, can you have a look and
perhaps approve it?

Thanks,

Martin

Re: SRA: don't drop clobbers

[Richard Biener]

On Thu, Nov 20, 2014 at 7:11 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Mon, Nov 03, 2014 at 10:46:49PM +0100, Marc Glisse wrote:
>> On Mon, 3 Nov 2014, Marc Glisse wrote:
>>
>> >On Mon, 3 Nov 2014, Martin Jambor wrote:
>> >
>> >>I just applied your patch on top of trunk revision 217032 on my
>> >
>> >Ah, that explains it, thanks. This patch is a follow-up to
>> >r217034. Still, I didn't expect the ICE you are seeing by applying
>> >this patch to older trunk, I'll try to reproduce that.
>>
>> It is TODO_update_address_taken that used to remove clobbers, and as
>> you said ESRA goes straight to TODO_update_ssa, which explains why
>> the clobbers caused trouble. In any case, after r217034, update_ssa
>> should handle clobbers much better. Could you take an other look
>> based on a more recent trunk, please?
>>
>
> Sorry for the delay.  Anyway, on the current trunk (i.e. Tuesday
> checkout) the patch works as expected, there are assignments from
> default definitions now and even though we do not warn as we should,
> the patch improves the generated code.  The function foo from the
> testcase is optimized to "return SR.1_2(D);" as soon as release_ssa
> now, whereas unpatched trunk leaves an undefined load even in the
> optimized dump.
>
> Thus, I like the patch and given that you posted it well before stage1
> end, I'd like to see it committed.  Richi, can you have a look and
> perhaps approve it?

Yes, the patch is ok.

Thanks,
Richard.

> Thanks,
>
> Martin
>