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

[Richard Biener <rguenther at suse dot de>]

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.

So I don't think the patch is good as-is.  Please consider reverting
if you already applied it.

Thanks,
Richard.