Re: [PATCH] Clear flow-sensitive info in phiopt (PR tree-optimization/67769)

[Marek Polacek <polacek at redhat dot com>]

On Thu, Oct 01, 2015 at 09:57:54AM +0200, Richard Biener wrote:
> On Wed, 30 Sep 2015, Marek Polacek wrote:
> 
> > Another instance of out of date SSA range info.  Before phiopt1 we had
> > 
> >   <bb 2>:
> >   if (N_2(D) >= 0)
> >     goto <bb 3>;
> >   else
> >     goto <bb 4>;
> > 
> >   <bb 3>:
> >   iftmp.0_3 = MIN_EXPR <N_2(D), 16>;
> > 
> >   <bb 4>:
> >   # iftmp.0_5 = PHI <0(2), iftmp.0_3(3)>
> >   value_4 = (short int) iftmp.0_5;
> >   return value_4;
> > 
> > and after phiop1:
> > 
> >   <bb 2>:
> >   iftmp.0_3 = MIN_EXPR <N_2(D), 16>;
> >   iftmp.0_6 = MAX_EXPR <iftmp.0_3, 0>;
> >   value_4 = (short int) iftmp.0_6;
> >   return value_4;
> > 
> > But the flow-sensitive info in this BB hasn't been cleared up.
> > 
> > This problem doesn't show up in GCC5 but might be latent there.
> > 
> > Bootstrapped/regtested on x86_64-linux, ok for trunk and 5 as well?
> > 
> > 2015-09-30  Marek Polacek  <polacek@redhat.com>
> > 
> > 	PR tree-optimization/67769
> > 	* tree-ssa-phiopt.c (tree_ssa_phiopt_worker): Call
> > 	reset_flow_sensitive_info_in_bb when changing the CFG.
> > 
> > 	* gcc.dg/torture/pr67769.c: New test.
> > 
> > diff --git gcc/testsuite/gcc.dg/torture/pr67769.c gcc/testsuite/gcc.dg/torture/pr67769.c
> > index e69de29..c1d17c3 100644
> > --- gcc/testsuite/gcc.dg/torture/pr67769.c
> > +++ gcc/testsuite/gcc.dg/torture/pr67769.c
> > @@ -0,0 +1,23 @@
> > +/* { dg-do run } */
> > +
> > +static int
> > +clamp (int x, int lo, int hi)
> > +{
> > +  return (x < lo) ? lo : ((x > hi) ? hi : x);
> > +}
> > +
> > +__attribute__ ((noinline))
> > +short
> > +foo (int N)
> > +{
> > +  short value = clamp (N, 0, 16);
> > +  return value;
> > +}
> > +
> > +int
> > +main ()
> > +{
> > +  if (foo (-5) != 0)
> > +    __builtin_abort ();
> > +  return 0;
> > +}
> > diff --git gcc/tree-ssa-phiopt.c gcc/tree-ssa-phiopt.c
> > index 37fdf28..101988a 100644
> > --- gcc/tree-ssa-phiopt.c
> > +++ gcc/tree-ssa-phiopt.c
> > @@ -338,6 +338,8 @@ tree_ssa_phiopt_worker (bool do_store_elim, bool do_hoist_loads)
> >  	  else if (minmax_replacement (bb, bb1, e1, e2, phi, arg0, arg1))
> >  	    cfgchanged = true;
> >  	}
> > +      if (cfgchanged)
> > +	reset_flow_sensitive_info_in_bb (bb);
> 
> That's a bit conservative.  I believe most PHI opt transforms should
> be fine as the conditionally executed blocks did not contain any
> stmts that prevail.  The merge PHI also should have valid range info.

Aha.  So would reseting the flow info at the end of conditional_replacement,
minmax_replacement, and abs_replacement work for you?  As in the untested
patch.  Or even somewhere else?

Yeah, I thought I'd rather be conservative here...
 
> So I don't think the patch is good as-is.  Please consider reverting
> if you already applied it.

Luckily I have not ;).

2015-10-01  Marek Polacek  <polacek@redhat.com>

	PR tree-optimization/67769
	* tree-ssa-phiopt.c (conditional_replacement): Call
	reset_flow_sensitive_info_in_bb.
	(minmax_replacement): Likewise.
	(abs_replacement): Likewise.

	* gcc.dg/torture/pr67769.c: New test.

diff --git gcc/testsuite/gcc.dg/torture/pr67769.c gcc/testsuite/gcc.dg/torture/pr67769.c
index e69de29..c1d17c3 100644
--- gcc/testsuite/gcc.dg/torture/pr67769.c
+++ gcc/testsuite/gcc.dg/torture/pr67769.c
@@ -0,0 +1,23 @@
+/* { dg-do run } */
+
+static int
+clamp (int x, int lo, int hi)
+{
+  return (x < lo) ? lo : ((x > hi) ? hi : x);
+}
+
+__attribute__ ((noinline))
+short
+foo (int N)
+{
+  short value = clamp (N, 0, 16);
+  return value;
+}
+
+int
+main ()
+{
+  if (foo (-5) != 0)
+    __builtin_abort ();
+  return 0;
+}
diff --git gcc/tree-ssa-phiopt.c gcc/tree-ssa-phiopt.c
index 37fdf28..697836a 100644
--- gcc/tree-ssa-phiopt.c
+++ gcc/tree-ssa-phiopt.c
@@ -646,6 +646,7 @@ conditional_replacement (basic_block cond_bb, basic_block middle_bb,
     }
 
   replace_phi_edge_with_variable (cond_bb, e1, phi, new_var);
+  reset_flow_sensitive_info_in_bb (cond_bb);
 
   /* Note that we optimized this PHI.  */
   return true;
@@ -1284,6 +1285,8 @@ minmax_replacement (basic_block cond_bb, basic_block middle_bb,
   gsi_insert_before (&gsi, new_stmt, GSI_NEW_STMT);
 
   replace_phi_edge_with_variable (cond_bb, e1, phi, result);
+  reset_flow_sensitive_info_in_bb (cond_bb);
+
   return true;
 }
 
@@ -1402,6 +1405,7 @@ abs_replacement (basic_block cond_bb, basic_block middle_bb,
     }
 
   replace_phi_edge_with_variable (cond_bb, e1, phi, result);
+  reset_flow_sensitive_info_in_bb (cond_bb);
 
   /* Note that we optimized this PHI.  */
   return true;

	Marek