On Thu, 3 Dec 2015, Jakub Jelinek wrote:

> Hi!
> 
> As mentioned in the PR, maybe_optimize_range_tests considers basic blocks
> with not just the final GIMPLE_COND (or for last_bb store feeding into PHI),
> but also assign stmts that don't trap, don't have side-effects and where
> the SSA_NAMEs they set are used only in their own bb.
> Now, if we decide to optimize some range test, we can change some conditions
> on previous bbs and that means we could execute some basic blocks that
> wouldn't be executed in the original program.  As the stmts don't set
> anything used in other bbs, they are most likely dead after the
> optimization, but the problem on the testcase is that because of the
> condition changes in previous bb we end up with incorrect value range
> for some SSA_NAME(s).  That can result in the miscompilation of the testcase
> on certain targets.
> 
> Fixed by resetting the value range info of such SSA_NAMEs.  I believe it
> shouldn't be a big deal, they will be mostly dead anyway.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2015-12-03  Jakub Jelinek  <ja...@redhat.com>
> 
>       PR tree-optimization/68671
>       * tree-ssa-reassoc.c (maybe_optimize_range_tests): For basic
>       blocks starting with the successor of first bb we've modified
>       and ending with last_bb, reset value ranges of all integral
>       SSA_NAMEs set in those basic blocks.
> 
>       * gcc.dg/pr68671.c: New test.
> 
> --- gcc/tree-ssa-reassoc.c.jj 2015-11-18 11:22:51.000000000 +0100
> +++ gcc/tree-ssa-reassoc.c    2015-12-03 18:12:08.915210122 +0100
> @@ -3204,7 +3204,7 @@ maybe_optimize_range_tests (gimple *stmt
>      any_changes = optimize_range_tests (ERROR_MARK, &ops);
>    if (any_changes)
>      {
> -      unsigned int idx;
> +      unsigned int idx, max_idx = 0;
>        /* update_ops relies on has_single_use predicates returning the
>        same values as it did during get_ops earlier.  Additionally it
>        never removes statements, only adds new ones and it should walk
> @@ -3220,6 +3220,7 @@ maybe_optimize_range_tests (gimple *stmt
>           {
>             tree new_op;
>  
> +           max_idx = idx;
>             stmt = last_stmt (bb);
>             new_op = update_ops (bbinfo[idx].op,
>                                  (enum tree_code)
> @@ -3289,6 +3290,10 @@ maybe_optimize_range_tests (gimple *stmt
>             && ops[bbinfo[idx].first_idx]->op != NULL_TREE)
>           {
>             gcond *cond_stmt = as_a <gcond *> (last_stmt (bb));
> +
> +           if (idx > max_idx)
> +             max_idx = idx;
> +
>             if (integer_zerop (ops[bbinfo[idx].first_idx]->op))
>               gimple_cond_make_false (cond_stmt);
>             else if (integer_onep (ops[bbinfo[idx].first_idx]->op))
> @@ -3305,6 +3310,30 @@ maybe_optimize_range_tests (gimple *stmt
>         if (bb == first_bb)
>           break;
>       }
> +
> +      /* The above changes could result in basic blocks after the first
> +      modified one, up to and including last_bb, to be executed even if
> +      they would not be in the original program.  If the value ranges of
> +      assignment lhs' in those bbs were dependent on the conditions
> +      guarding those basic blocks which now can change, the VRs might
> +      be incorrect.  As no_side_effect_bb should ensure those SSA_NAMEs
> +      are only used within the same bb, it should be not a big deal if
> +      we just reset all the VRs in those bbs.  See PR68671.  */
> +      for (bb = last_bb, idx = 0; idx < max_idx; bb = single_pred (bb), 
> idx++)
> +     {
> +       gimple_stmt_iterator gsi;
> +       for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
> +         {
> +           gimple *g = gsi_stmt (gsi);
> +           if (!is_gimple_assign (g))
> +             continue;
> +           tree lhs = gimple_assign_lhs (g);
> +           if (TREE_CODE (lhs) != SSA_NAME)
> +             continue;
> +           if (INTEGRAL_TYPE_P (TREE_TYPE (lhs)))
> +             SSA_NAME_RANGE_INFO (lhs) = NULL;

Please use

     reset_flow_sensitive_info (lhs);

Ok with that change.

Thanks,
Richard.

> +         }
> +     }
>      }
>  }
>  
> --- gcc/testsuite/gcc.dg/pr68671.c.jj 2015-12-03 18:19:24.769104484 +0100
> +++ gcc/testsuite/gcc.dg/pr68671.c    2015-12-03 18:19:07.000000000 +0100
> @@ -0,0 +1,23 @@
> +/* PR tree-optimization/68671 */
> +/* { dg-do run } */
> +/* { dg-options " -O2 -fno-tree-dce" } */
> +
> +volatile int a = -1;
> +volatile int b;
> +
> +static inline int
> +fn1 (signed char p1, int p2)
> +{
> +  return (p1 < 0) || (p1 > (1 >> p2)) ? 0 : (p1 << 1);
> +}
> +
> +int
> +main ()
> +{
> +  signed char c = a;
> +  b = fn1 (c, 1);
> +  c = ((128 | c) < 0 ? 1 : 0);
> +  if (c != 1)
> +    __builtin_abort ();
> +  return 0;
> +}
> 
>       Jakub
> 
> 

-- 
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 
21284 (AG Nuernberg)

Reply via email to