Patchwork Fix substitute_and_fold ICE (PR tree-optimization/55331)

login
register
mail settings
Submitter Jakub Jelinek
Date Nov. 15, 2012, 8:09 p.m.
Message ID <20121115200907.GC1886@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/199396/
State New
Headers show

Comments

Jakub Jelinek - Nov. 15, 2012, 8:09 p.m.
Hi!

On the following testcase substitute_and_fold ICEs because memmove
of length 1 on an empty class is optimized away, and this function wasn't
prepared to handle that.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk/4.7?

2012-11-15  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/55331
	* tree-ssa-propagate.c (substitute_and_fold): Handle fold_stmt
	turning a stmt into nothing.

	* g++.dg/opt/pr55331.C: New test.


	Jakub
Richard Guenther - Nov. 26, 2012, 3:24 p.m.
On Thu, Nov 15, 2012 at 9:09 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> On the following testcase substitute_and_fold ICEs because memmove
> of length 1 on an empty class is optimized away, and this function wasn't
> prepared to handle that.
>
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk/4.7?

I think the bug is that the stmt is removed.  fold_stmt is not supposed to
"remove" the stmt in any case - it may replace it with a gimple_nop at most.

Thus, gimplify_and_update_call_from_tree is at fault here.

I am testing a patch that fixes it.

Thanks,
Richard.

> 2012-11-15  Jakub Jelinek  <jakub@redhat.com>
>
>         PR tree-optimization/55331
>         * tree-ssa-propagate.c (substitute_and_fold): Handle fold_stmt
>         turning a stmt into nothing.
>
>         * g++.dg/opt/pr55331.C: New test.
>
> --- gcc/tree-ssa-propagate.c.jj 2012-11-02 09:01:55.000000000 +0100
> +++ gcc/tree-ssa-propagate.c    2012-11-15 12:27:37.218543417 +0100
> @@ -1094,7 +1094,7 @@ substitute_and_fold (ssa_prop_get_value_
>           gimple stmt = gsi_stmt (i);
>           gimple old_stmt;
>           enum gimple_code code = gimple_code (stmt);
> -         gimple_stmt_iterator oldi;
> +         gimple_stmt_iterator oldi, nexti;
>
>           oldi = i;
>           if (do_dce)
> @@ -1147,6 +1147,8 @@ substitute_and_fold (ssa_prop_get_value_
>             }
>
>           old_stmt = stmt;
> +         nexti = oldi;
> +         gsi_next (&nexti);
>
>           /* Some statements may be simplified using propagator
>              specific information.  Do this before propagating
> @@ -1171,36 +1173,63 @@ substitute_and_fold (ssa_prop_get_value_
>           /* Now cleanup.  */
>           if (did_replace)
>             {
> -             stmt = gsi_stmt (oldi);
> -
> -              /* If we cleaned up EH information from the statement,
> -                 remove EH edges.  */
> -             if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt))
> -               gimple_purge_dead_eh_edges (bb);
> -
> -              if (is_gimple_assign (stmt)
> -                  && (get_gimple_rhs_class (gimple_assign_rhs_code (stmt))
> -                      == GIMPLE_SINGLE_RHS))
> -              {
> -                tree rhs = gimple_assign_rhs1 (stmt);
> -
> -                if (TREE_CODE (rhs) == ADDR_EXPR)
> -                  recompute_tree_invariant_for_addr_expr (rhs);
> -              }
> +             /* old_stmt might have been also replaced by nothing,
> +                in that case set stmt to NULL.  */
> +             if (gsi_end_p (oldi))
> +               {
> +                 gcc_checking_assert (gsi_end_p (nexti));
> +                 stmt = NULL;
> +               }
> +             else
> +               {
> +                 stmt = gsi_stmt (oldi);
> +                 if (!gsi_end_p (nexti) && gsi_stmt (nexti) == stmt)
> +                   stmt = NULL;
> +               }
>
> -             /* Determine what needs to be done to update the SSA form.  */
> -             update_stmt (stmt);
> -             if (!is_gimple_debug (stmt))
> -               something_changed = true;
> +             if (stmt == NULL)
> +               {
> +                 if (maybe_clean_eh_stmt (old_stmt))
> +                   gimple_purge_dead_eh_edges (bb);
> +                 something_changed = true;
> +               }
> +             else
> +               {
> +                 /* If we cleaned up EH information from the statement,
> +                    remove EH edges.  */
> +                 if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt))
> +                   gimple_purge_dead_eh_edges (bb);
> +
> +                 if (is_gimple_assign (stmt)
> +                     && (get_gimple_rhs_class (gimple_assign_rhs_code (stmt))
> +                         == GIMPLE_SINGLE_RHS))
> +                   {
> +                     tree rhs = gimple_assign_rhs1 (stmt);
> +
> +                     if (TREE_CODE (rhs) == ADDR_EXPR)
> +                       recompute_tree_invariant_for_addr_expr (rhs);
> +                   }
> +
> +                 /* Determine what needs to be done to update the SSA
> +                    form.  */
> +                 update_stmt (stmt);
> +                 if (!is_gimple_debug (stmt))
> +                   something_changed = true;
> +               }
>             }
>
>           if (dump_file && (dump_flags & TDF_DETAILS))
>             {
>               if (did_replace)
>                 {
> -                 fprintf (dump_file, "Folded into: ");
> -                 print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
> -                 fprintf (dump_file, "\n");
> +                 if (stmt == NULL)
> +                   fprintf (dump_file, "Folded into nothing\n");
> +                 else
> +                   {
> +                     fprintf (dump_file, "Folded into: ");
> +                     print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
> +                     fprintf (dump_file, "\n");
> +                   }
>                 }
>               else
>                 fprintf (dump_file, "Not folded\n");
> --- gcc/testsuite/g++.dg/opt/pr55331.C.jj       2012-11-15 12:53:26.893631190 +0100
> +++ gcc/testsuite/g++.dg/opt/pr55331.C  2012-11-15 12:53:52.482504446 +0100
> @@ -0,0 +1,14 @@
> +// PR tree-optimization/55331
> +// { dg-do compile }
> +// { dg-options "-O2 -fno-tree-fre" }
> +
> +struct A {};
> +
> +void
> +foo (A *p, bool x)
> +{
> +  A a;
> +  char *e = (char *) (&a + 1);
> +  if (x)
> +    __builtin_memmove (p, &a, e - (char *) &a);
> +}
>
>         Jakub
Jakub Jelinek - Nov. 26, 2012, 3:27 p.m.
On Mon, Nov 26, 2012 at 04:24:41PM +0100, Richard Biener wrote:
> On Thu, Nov 15, 2012 at 9:09 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On the following testcase substitute_and_fold ICEs because memmove
> > of length 1 on an empty class is optimized away, and this function wasn't
> > prepared to handle that.
> >
> > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> > trunk/4.7?
> 
> I think the bug is that the stmt is removed.  fold_stmt is not supposed to
> "remove" the stmt in any case - it may replace it with a gimple_nop at most.
> 
> Thus, gimplify_and_update_call_from_tree is at fault here.
> 
> I am testing a patch that fixes it.

Note that fold_stmt_1 also has code to handle a call folding into nothing,
so perhaps that could be removed too if you change it to fold into a nop
instead.

	Jakub
Richard Guenther - Nov. 27, 2012, 10:10 a.m.
On Mon, Nov 26, 2012 at 4:27 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Nov 26, 2012 at 04:24:41PM +0100, Richard Biener wrote:
>> On Thu, Nov 15, 2012 at 9:09 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On the following testcase substitute_and_fold ICEs because memmove
>> > of length 1 on an empty class is optimized away, and this function wasn't
>> > prepared to handle that.
>> >
>> > Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
>> > trunk/4.7?
>>
>> I think the bug is that the stmt is removed.  fold_stmt is not supposed to
>> "remove" the stmt in any case - it may replace it with a gimple_nop at most.
>>
>> Thus, gimplify_and_update_call_from_tree is at fault here.
>>
>> I am testing a patch that fixes it.
>
> Note that fold_stmt_1 also has code to handle a call folding into nothing,
> so perhaps that could be removed too if you change it to fold into a nop
> instead.

Yeah, I'm testing a patch to remove that.

Richard.

>         Jakub

Patch

--- gcc/tree-ssa-propagate.c.jj	2012-11-02 09:01:55.000000000 +0100
+++ gcc/tree-ssa-propagate.c	2012-11-15 12:27:37.218543417 +0100
@@ -1094,7 +1094,7 @@  substitute_and_fold (ssa_prop_get_value_
 	  gimple stmt = gsi_stmt (i);
 	  gimple old_stmt;
 	  enum gimple_code code = gimple_code (stmt);
-	  gimple_stmt_iterator oldi;
+	  gimple_stmt_iterator oldi, nexti;
 
 	  oldi = i;
 	  if (do_dce)
@@ -1147,6 +1147,8 @@  substitute_and_fold (ssa_prop_get_value_
 	    }
 
 	  old_stmt = stmt;
+	  nexti = oldi;
+	  gsi_next (&nexti);
 
 	  /* Some statements may be simplified using propagator
 	     specific information.  Do this before propagating
@@ -1171,36 +1173,63 @@  substitute_and_fold (ssa_prop_get_value_
 	  /* Now cleanup.  */
 	  if (did_replace)
 	    {
-	      stmt = gsi_stmt (oldi);
-
-              /* If we cleaned up EH information from the statement,
-                 remove EH edges.  */
-	      if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt))
-		gimple_purge_dead_eh_edges (bb);
-
-              if (is_gimple_assign (stmt)
-                  && (get_gimple_rhs_class (gimple_assign_rhs_code (stmt))
-                      == GIMPLE_SINGLE_RHS))
-              {
-                tree rhs = gimple_assign_rhs1 (stmt);
-
-                if (TREE_CODE (rhs) == ADDR_EXPR)
-                  recompute_tree_invariant_for_addr_expr (rhs);
-              }
+	      /* old_stmt might have been also replaced by nothing,
+		 in that case set stmt to NULL.  */
+	      if (gsi_end_p (oldi))
+		{
+		  gcc_checking_assert (gsi_end_p (nexti));
+		  stmt = NULL;
+		}
+	      else
+		{
+		  stmt = gsi_stmt (oldi);
+		  if (!gsi_end_p (nexti) && gsi_stmt (nexti) == stmt)
+		    stmt = NULL;
+		}
 
-	      /* Determine what needs to be done to update the SSA form.  */
-	      update_stmt (stmt);
-	      if (!is_gimple_debug (stmt))
-		something_changed = true;
+	      if (stmt == NULL)
+		{
+		  if (maybe_clean_eh_stmt (old_stmt))
+		    gimple_purge_dead_eh_edges (bb);
+		  something_changed = true;
+		}
+	      else
+		{
+		  /* If we cleaned up EH information from the statement,
+		     remove EH edges.  */
+		  if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt))
+		    gimple_purge_dead_eh_edges (bb);
+
+		  if (is_gimple_assign (stmt)
+		      && (get_gimple_rhs_class (gimple_assign_rhs_code (stmt))
+			  == GIMPLE_SINGLE_RHS))
+		    {
+		      tree rhs = gimple_assign_rhs1 (stmt);
+
+		      if (TREE_CODE (rhs) == ADDR_EXPR)
+			recompute_tree_invariant_for_addr_expr (rhs);
+		    }
+
+		  /* Determine what needs to be done to update the SSA
+		     form.  */
+		  update_stmt (stmt);
+		  if (!is_gimple_debug (stmt))
+		    something_changed = true;
+		}
 	    }
 
 	  if (dump_file && (dump_flags & TDF_DETAILS))
 	    {
 	      if (did_replace)
 		{
-		  fprintf (dump_file, "Folded into: ");
-		  print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
-		  fprintf (dump_file, "\n");
+		  if (stmt == NULL)
+		    fprintf (dump_file, "Folded into nothing\n");
+		  else
+		    {
+		      fprintf (dump_file, "Folded into: ");
+		      print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
+		      fprintf (dump_file, "\n");
+		    }
 		}
 	      else
 		fprintf (dump_file, "Not folded\n");
--- gcc/testsuite/g++.dg/opt/pr55331.C.jj	2012-11-15 12:53:26.893631190 +0100
+++ gcc/testsuite/g++.dg/opt/pr55331.C	2012-11-15 12:53:52.482504446 +0100
@@ -0,0 +1,14 @@ 
+// PR tree-optimization/55331
+// { dg-do compile }
+// { dg-options "-O2 -fno-tree-fre" }
+
+struct A {};
+
+void
+foo (A *p, bool x)
+{
+  A a;
+  char *e = (char *) (&a + 1);
+  if (x)
+    __builtin_memmove (p, &a, e - (char *) &a);
+}