Re: [PATCH] Fix cond updating in tree-ssa-dom (PR tree-optimization/77454)

[Jakub Jelinek]

On Tue, Sep 13, 2016 at 04:02:17PM +0200, Richard Biener wrote:
> Yes please, this should be safe.  All of the above do not look at immediate 
> uses but at most use-def links.

Ok, here is what I've committed after another bootstrap/regtest:

2016-09-13  Jakub Jelinek  

PR tree-optimization/77454
* tree-ssa-dom.c (optimize_stmt): Set modified flag on stmt after
changing GIMPLE_COND.  Move update_stmt_if_modified call after this.
Formatting fix.

* gcc.dg/pr77454.c: New test.

--- gcc/tree-ssa-dom.c.jj   2016-04-28 17:26:05.428203966 +0200
+++ gcc/tree-ssa-dom.c  2016-09-13 16:25:34.194491591 +0200
@@ -1923,12 +1923,11 @@ optimize_stmt (basic_block bb, gimple_st
 {
   tree val = NULL;
 
-  update_stmt_if_modified (stmt);
-
   if (gimple_code (stmt) == GIMPLE_COND)
 val = fold_binary_loc (gimple_location (stmt),
-  gimple_cond_code (stmt), boolean_type_node,
-   gimple_cond_lhs (stmt),  gimple_cond_rhs (stmt));
+  gimple_cond_code (stmt), boolean_type_node,
+  gimple_cond_lhs (stmt),
+  gimple_cond_rhs (stmt));
   else if (gswitch *swtch_stmt = dyn_cast  (stmt))
val = gimple_switch_index (swtch_stmt);
 
@@ -1946,6 +1945,8 @@ optimize_stmt (basic_block bb, gimple_st
gimple_cond_make_true (as_a  (stmt));
  else
gcc_unreachable ();
+
+ gimple_set_modified (stmt, true);
}
 
  /* Further simplifications may be possible.  */
@@ -1953,6 +1954,8 @@ optimize_stmt (basic_block bb, gimple_st
}
}
 
+  update_stmt_if_modified (stmt);
+
   /* If we simplified a statement in such a way as to be shown that it
 cannot trap, update the eh information and the cfg to match.  */
   if (maybe_clean_or_replace_eh_stmt (old_stmt, stmt))
--- gcc/testsuite/gcc.dg/pr77454.c.jj   2016-09-13 16:24:53.662977245 +0200
+++ gcc/testsuite/gcc.dg/pr77454.c  2016-09-13 16:24:53.662977245 +0200
@@ -0,0 +1,28 @@
+/* PR tree-optimization/77454 */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+void
+foo (unsigned char x, char y)
+{
+  while (x != 0)
+{
+  unsigned char *a = 
+  int b;
+
+  if (y != 0)
+   a = (unsigned char *) 
+  else if (y + 1 != 0)
+   a = (unsigned char *) 
+  for (x = 0; x < 1; ++x)
+   b = 0;
+  for (y = 0; y < 3; ++y)
+   {
+ y = !!y;
+ if (y != 0)
+   x = y;
+   }
+  if ((b != 0 ? -1 : *a) < (y = b))
+   b = 1;
+}
+}

Jakub

Re: [PATCH] Fix cond updating in tree-ssa-dom (PR tree-optimization/77454)

[Richard Biener]

On September 13, 2016 3:32:33 PM GMT+02:00, Jakub Jelinek  
wrote:
>On Tue, Sep 13, 2016 at 03:21:47PM +0200, Richard Biener wrote:
>> > The following testcase ICEs because SSA_NAME IMM links are broken.
>> > I've tracked it to DOM's optimize_stmt, a GIMPLE_COND in there is
>> > changed, marked as modified, then in optimize_stmt
>> >   if (gimple_modified_p (stmt) || modified_p)
>> > {
>> >   tree val = NULL;
>> >
>> >   update_stmt_if_modified (stmt);
>> > and a few lines later changed again:
>> >   if (gimple_code (stmt) == GIMPLE_COND)
>> > {
>> >   if (integer_zerop (val))
>> > gimple_cond_make_false (as_a  (stmt));
>> >   else if (integer_onep (val))
>> > gimple_cond_make_true (as_a  (stmt));
>> > without update_stmt.  As this is a function which
>update_stmt_if_modified
>> > a few lines before, I think it is fine to update_stmt it
>immediately.
>> >
>> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
>trunk?
>> 
>> Ok if it doesn't do the update twice this way (we do it the "fancy
>way" to
>> only update stmts we modified and do it only once).
>
>It does update it twice this way, the
>   update_stmt_if_modified (stmt);
>above is done first and then the newly added update_stmt again.
>
>There is
>  if (gimple_code (stmt) == GIMPLE_COND)
>val = fold_binary_loc (gimple_location (stmt),
>   gimple_cond_code (stmt), boolean_type_node,
>  gimple_cond_lhs (stmt),  gimple_cond_rhs (stmt));
>  else if (gswitch *swtch_stmt = dyn_cast  (stmt))
>val = gimple_switch_index (swtch_stmt);
>
>  if (val && TREE_CODE (val) == INTEGER_CST)
>{
>  retval = find_taken_edge (bb, val);
>and gimple_cond_make_{false,true} in between.  If none of these care
>about
>the stmt being updated (gimple_cond_make_{false,true} and
>gimple_switch_index is surely ok, dyn_cast too, not 100% sure about
>fold_binary (if it can't use match.pd, though on the other side it is
>not
>passed the stmt, but only its arguments), or find_taken_edge (most
>likely
>ok)), then I could perhaps change the patch to do gimple_set_modified
>(stmt,
>true); where I've added the update_stmt, and move the
>update_stmt_if_modified call after the if (val && TREE_CODE (val) ==
>INTEGER_CST) { ... } stmt.

Yes please, this should be safe.  All of the above do not look at immediate 
uses but at most use-def links.

Richard.

>   Jakub

Re: [PATCH] Fix cond updating in tree-ssa-dom (PR tree-optimization/77454)

[Jakub Jelinek]

On Tue, Sep 13, 2016 at 03:21:47PM +0200, Richard Biener wrote:
> > The following testcase ICEs because SSA_NAME IMM links are broken.
> > I've tracked it to DOM's optimize_stmt, a GIMPLE_COND in there is
> > changed, marked as modified, then in optimize_stmt
> >   if (gimple_modified_p (stmt) || modified_p)
> > {
> >   tree val = NULL;
> >
> >   update_stmt_if_modified (stmt);
> > and a few lines later changed again:
> >   if (gimple_code (stmt) == GIMPLE_COND)
> > {
> >   if (integer_zerop (val))
> > gimple_cond_make_false (as_a  (stmt));
> >   else if (integer_onep (val))
> > gimple_cond_make_true (as_a  (stmt));
> > without update_stmt.  As this is a function which update_stmt_if_modified
> > a few lines before, I think it is fine to update_stmt it immediately.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> Ok if it doesn't do the update twice this way (we do it the "fancy way" to
> only update stmts we modified and do it only once).

It does update it twice this way, the
   update_stmt_if_modified (stmt);
above is done first and then the newly added update_stmt again.

There is
  if (gimple_code (stmt) == GIMPLE_COND)
val = fold_binary_loc (gimple_location (stmt),
   gimple_cond_code (stmt), boolean_type_node,
   gimple_cond_lhs (stmt),  gimple_cond_rhs (stmt));
  else if (gswitch *swtch_stmt = dyn_cast  (stmt))
val = gimple_switch_index (swtch_stmt);

  if (val && TREE_CODE (val) == INTEGER_CST)
{
  retval = find_taken_edge (bb, val);
and gimple_cond_make_{false,true} in between.  If none of these care about
the stmt being updated (gimple_cond_make_{false,true} and
gimple_switch_index is surely ok, dyn_cast too, not 100% sure about
fold_binary (if it can't use match.pd, though on the other side it is not
passed the stmt, but only its arguments), or find_taken_edge (most likely
ok)), then I could perhaps change the patch to do gimple_set_modified (stmt,
true); where I've added the update_stmt, and move the
update_stmt_if_modified call after the if (val && TREE_CODE (val) ==
INTEGER_CST) { ... } stmt.

Jakub

Re: [PATCH] Fix cond updating in tree-ssa-dom (PR tree-optimization/77454)

[Richard Biener]

On Tue, Sep 13, 2016 at 9:46 AM, Jakub Jelinek  wrote:
> Hi!
>
> The following testcase ICEs because SSA_NAME IMM links are broken.
> I've tracked it to DOM's optimize_stmt, a GIMPLE_COND in there is
> changed, marked as modified, then in optimize_stmt
>   if (gimple_modified_p (stmt) || modified_p)
> {
>   tree val = NULL;
>
>   update_stmt_if_modified (stmt);
> and a few lines later changed again:
>   if (gimple_code (stmt) == GIMPLE_COND)
> {
>   if (integer_zerop (val))
> gimple_cond_make_false (as_a  (stmt));
>   else if (integer_onep (val))
> gimple_cond_make_true (as_a  (stmt));
> without update_stmt.  As this is a function which update_stmt_if_modified
> a few lines before, I think it is fine to update_stmt it immediately.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok if it doesn't do the update twice this way (we do it the "fancy way" to
only update stmts we modified and do it only once).

Richard.

> 2016-09-13  Jakub Jelinek  
>
> PR tree-optimization/77454
> * tree-ssa-dom.c (optimize_stmt): Call update_stmt after changing
> GIMPLE_COND.  Formatting fix.
>
> * gcc.dg/pr77454.c: New test.
>
> --- gcc/tree-ssa-dom.c.jj   2016-07-22 15:55:30.0 +0200
> +++ gcc/tree-ssa-dom.c  2016-09-09 12:29:28.006188533 +0200
> @@ -1927,8 +1927,9 @@ optimize_stmt (basic_block bb, gimple_st
>
>if (gimple_code (stmt) == GIMPLE_COND)
>  val = fold_binary_loc (gimple_location (stmt),
> -  gimple_cond_code (stmt), boolean_type_node,
> -   gimple_cond_lhs (stmt),  gimple_cond_rhs (stmt));
> +  gimple_cond_code (stmt), boolean_type_node,
> +  gimple_cond_lhs (stmt),
> +  gimple_cond_rhs (stmt));
>else if (gswitch *swtch_stmt = dyn_cast  (stmt))
> val = gimple_switch_index (swtch_stmt);
>
> @@ -1946,6 +1947,8 @@ optimize_stmt (basic_block bb, gimple_st
> gimple_cond_make_true (as_a  (stmt));
>   else
> gcc_unreachable ();
> +
> + update_stmt (stmt);
> }
>
>   /* Further simplifications may be possible.  */
> --- gcc/testsuite/gcc.dg/pr77454.c.jj   2016-09-09 12:34:47.540537483 +0200
> +++ gcc/testsuite/gcc.dg/pr77454.c  2016-09-09 12:32:47.0 +0200
> @@ -0,0 +1,28 @@
> +/* PR tree-optimization/77454 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +void
> +foo (unsigned char x, char y)
> +{
> +  while (x != 0)
> +{
> +  unsigned char *a = 
> +  int b;
> +
> +  if (y != 0)
> +   a = (unsigned char *) 
> +  else if (y + 1 != 0)
> +   a = (unsigned char *) 
> +  for (x = 0; x < 1; ++x)
> +   b = 0;
> +  for (y = 0; y < 3; ++y)
> +   {
> + y = !!y;
> + if (y != 0)
> +   x = y;
> +   }
> +  if ((b != 0 ? -1 : *a) < (y = b))
> +   b = 1;
> +}
> +}
>
> Jakub