On Thu, 1 Oct 2015, Marek Polacek wrote:
> 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 <pola...@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?
No, this looks fine.
Thanks,
Richard.
> 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 <pola...@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
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
