Re: [PATCH] Robustify simplify_truth_ops_using_ranges (PR tree-optimization/37662, PR tree-optimization/37663)

["Richard Guenther" <richard dot guenther at gmail dot com>]

On Tue, Sep 30, 2008 at 3:03 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> The newly added simplify_truth_ops_using_ranges seems to be too fragile,
> assumes the first argument must be always a SSA_NAME and only very few
> possibilities for the second argument.  The first assumption isn't true
> e.g. when ccp or other passes substituted one of the truth op operands
> but fab hasn't happened yet to swap the commutative operands to the
> preferred order.  The second might happen because of TREE_OVERFLOW prevented
> folding, or similar reasons.  It seems only fold and 3 further cases swap
> commutative operands, while we could do that in ccp, relying on it in vrp
> is too risky.  The patch swaps the operands itself (just in op0/op1
> variables) if needed, and bails out if get_value_range would be called on
> non-SSA_NAME, furthermore replaces the two gcc_asserts with a conditional
> return false.  The patch looks bigger than it really is, most of the changes
> are caused by reindentation.
>
> Ok for trunk if bootstrap/regtest succeeds?

Ok.

Thanks,
Richard.

> 2008-09-30  Jakub Jelinek  <jakub@redhat.com>
>
>        PR tree-optimization/37662
>        PR tree-optimization/37663
>        * tree-vrp.c (simplify_truth_ops_using_ranges): Swap operands if needed
>        (and not TRUTH_NOT_EXPR).  Don't call get_value_range with non-SSA_NAME.
>        Don't assert operands have been folded, instead just bail out.
>
>        * gcc.c-torture/compile/pr37662.c: New test.
>        * gcc.dg/pr37663.c: New test.
>
> --- gcc/tree-vrp.c.jj   2008-09-16 16:50:49.000000000 +0200
> +++ gcc/tree-vrp.c      2008-09-30 13:50:58.000000000 +0200
> @@ -6304,9 +6304,27 @@ simplify_truth_ops_using_ranges (gimple_
>   bool need_conversion;
>
>   op0 = gimple_assign_rhs1 (stmt);
> -  vr = get_value_range (op0);
> +  if (rhs_code == TRUTH_NOT_EXPR)
> +    {
> +      rhs_code = NE_EXPR;
> +      op1 = build_int_cst (TREE_TYPE (op0), 1);
> +    }
> +  else
> +    {
> +      op1 = gimple_assign_rhs2 (stmt);
> +      if (tree_swap_operands_p (op0, op1, true))
> +       {
> +         op0 = op1;
> +         op1 = gimple_assign_rhs1 (stmt);
> +       }
> +    }
> +
>   if (TYPE_PRECISION (TREE_TYPE (op0)) != 1)
>     {
> +      if (TREE_CODE (op0) != SSA_NAME)
> +       return false;
> +      vr = get_value_range (op0);
> +
>       val = compare_range_with_value (GE_EXPR, vr, integer_zero_node, &sop);
>       if (!val || !integer_onep (val))
>         return false;
> @@ -6316,48 +6334,46 @@ simplify_truth_ops_using_ranges (gimple_
>         return false;
>     }
>
> -  if (rhs_code == TRUTH_NOT_EXPR)
> +  /* Reduce number of cases to handle.  */
> +  if (is_gimple_min_invariant (op1))
>     {
> -      rhs_code = NE_EXPR;
> -      op1 = build_int_cst (TREE_TYPE (op0), 1);
> +      /* Exclude anything that should have been already folded.  */
> +      if (rhs_code != EQ_EXPR
> +         && rhs_code != NE_EXPR
> +         && rhs_code != TRUTH_XOR_EXPR)
> +       return false;
> +
> +      if (!integer_zerop (op1)
> +         && !integer_onep (op1)
> +         && !integer_all_onesp (op1))
> +       return false;
> +
> +      /* Limit the number of cases we have to consider.  */
> +      if (rhs_code == EQ_EXPR)
> +       {
> +         rhs_code = NE_EXPR;
> +         op1 = fold_unary (TRUTH_NOT_EXPR, TREE_TYPE (op1), op1);
> +       }
>     }
>   else
>     {
> -      op1 = gimple_assign_rhs2 (stmt);
> +      /* Punt on A == B as there is no BIT_XNOR_EXPR.  */
> +      if (rhs_code == EQ_EXPR)
> +       return false;
>
> -      /* Reduce number of cases to handle.  */
> -      if (is_gimple_min_invariant (op1))
> +      if (TYPE_PRECISION (TREE_TYPE (op1)) != 1)
>        {
> -          /* Exclude anything that should have been already folded.  */
> -         gcc_assert (rhs_code == EQ_EXPR || rhs_code == NE_EXPR
> -                     || rhs_code == TRUTH_XOR_EXPR);
> -         gcc_assert (integer_zerop (op1) || integer_onep (op1)
> -                     || integer_all_onesp (op1));
> +         if (TREE_CODE (op1) != SSA_NAME)
> +           return false;
> +         vr = get_value_range (op1);
>
> -         /* Limit the number of cases we have to consider.  */
> -         if (rhs_code == EQ_EXPR)
> -           {
> -             rhs_code = NE_EXPR;
> -             op1 = fold_unary (TRUTH_NOT_EXPR, TREE_TYPE (op1), op1);
> -           }
> -       }
> -      else
> -       {
> -         /* Punt on A == B as there is no BIT_XNOR_EXPR.  */
> -         if (rhs_code == EQ_EXPR)
> +         val = compare_range_with_value (GE_EXPR, vr, integer_zero_node, &sop);
> +         if (!val || !integer_onep (val))
>            return false;
>
> -         if (TYPE_PRECISION (TREE_TYPE (op1)) != 1)
> -           {
> -             vr = get_value_range (op1);
> -             val = compare_range_with_value (GE_EXPR, vr, integer_zero_node, &sop);
> -             if (!val || !integer_onep (val))
> -               return false;
> -
> -             val = compare_range_with_value (LE_EXPR, vr, integer_one_node, &sop);
> -             if (!val || !integer_onep (val))
> -               return false;
> -           }
> +         val = compare_range_with_value (LE_EXPR, vr, integer_one_node, &sop);
> +         if (!val || !integer_onep (val))
> +           return false;
>        }
>     }
>
> --- gcc/testsuite/gcc.c-torture/compile/pr37662.c.jj    2008-09-30 13:57:08.000000000 +0200
> +++ gcc/testsuite/gcc.c-torture/compile/pr37662.c       2008-09-30 13:56:08.000000000 +0200
> @@ -0,0 +1,15 @@
> +/* PR tree-optimization/37662 */
> +
> +extern int baz (void);
> +
> +static int
> +foo (void)
> +{
> +  return 1;
> +}
> +
> +int
> +bar (void)
> +{
> +  return foo () >= 1 ^ (baz () || 0) || 0;
> +}
> --- gcc/testsuite/gcc.dg/pr37663.c.jj   2008-09-30 13:56:43.000000000 +0200
> +++ gcc/testsuite/gcc.dg/pr37663.c      2008-09-30 13:55:33.000000000 +0200
> @@ -0,0 +1,15 @@
> +/* PR tree-optimization/37663 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fwrapv" } */
> +
> +extern void bar (void);
> +
> +void
> +foo (int x)
> +{
> +  x = 1 >= x;
> +  int y = -1885403717;
> +  x = x + (x != y * y);
> +  if (x)
> +    bar ();
> +}
>
>        Jakub
>