Adapt one fold-const optimization for vectors

On Tue, 30 Oct 2012, Marek Polacek wrote:

> On Tue, Oct 30, 2012 at 08:05:13AM +0100, Marc Glisse wrote:
>> Hello,
>>
>> one more optimization that needed help for vectors, it crashed on
>> (x<y)<0. Because of PR 55001, testcases are awkward to add (I could
>> do a x86-only one if needed).
>>
>> bootstrap+testsuite.
>>
>> 2012-10-30  Marc Glisse  <marc.glisse@inria.fr>
>>
>> 	* fold-const.c (fold_binary_op_with_conditional_arg): Handle vectors.
>> 	(fold_binary_loc): call it for VEC_COND_EXPR.
>
> Patch missing?

Indeed, thanks for the notice.

Here it is.

On Tue, Oct 30, 2012 at 1:14 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Tue, 30 Oct 2012, Marek Polacek wrote:
>
>> On Tue, Oct 30, 2012 at 08:05:13AM +0100, Marc Glisse wrote:
>>>
>>> Hello,
>>>
>>> one more optimization that needed help for vectors, it crashed on
>>> (x<y)<0. Because of PR 55001, testcases are awkward to add (I could
>>> do a x86-only one if needed).
>>>
>>> bootstrap+testsuite.
>>>
>>> 2012-10-30  Marc Glisse  <marc.glisse@inria.fr>
>>>
>>>         * fold-const.c (fold_binary_op_with_conditional_arg): Handle
>>> vectors.
>>>         (fold_binary_loc): call it for VEC_COND_EXPR.
>>
>>
>> Patch missing?
>
>
> Indeed, thanks for the notice.

Ok.

Thanks,
Richard.

> Here it is.
>
> --
> Marc Glisse
> Index: fold-const.c
> ===================================================================
> --- fold-const.c        (revision 192976)
> +++ fold-const.c        (working copy)
> @@ -5952,42 +5952,47 @@ static tree
>  fold_binary_op_with_conditional_arg (location_t loc,
>                                      enum tree_code code,
>                                      tree type, tree op0, tree op1,
>                                      tree cond, tree arg, int cond_first_p)
>  {
>    tree cond_type = cond_first_p ? TREE_TYPE (op0) : TREE_TYPE (op1);
>    tree arg_type = cond_first_p ? TREE_TYPE (op1) : TREE_TYPE (op0);
>    tree test, true_value, false_value;
>    tree lhs = NULL_TREE;
>    tree rhs = NULL_TREE;
> +  enum tree_code cond_code = COND_EXPR;
>
> -  if (TREE_CODE (cond) == COND_EXPR)
> +  if (TREE_CODE (cond) == COND_EXPR
> +      || TREE_CODE (cond) == VEC_COND_EXPR)
>      {
>        test = TREE_OPERAND (cond, 0);
>        true_value = TREE_OPERAND (cond, 1);
>        false_value = TREE_OPERAND (cond, 2);
>        /* If this operand throws an expression, then it does not make
>          sense to try to perform a logical or arithmetic operation
>          involving it.  */
>        if (VOID_TYPE_P (TREE_TYPE (true_value)))
>         lhs = true_value;
>        if (VOID_TYPE_P (TREE_TYPE (false_value)))
>         rhs = false_value;
>      }
>    else
>      {
>        tree testtype = TREE_TYPE (cond);
>        test = cond;
>        true_value = constant_boolean_node (true, testtype);
>        false_value = constant_boolean_node (false, testtype);
>      }
>
> +  if (TREE_CODE (TREE_TYPE (test)) == VECTOR_TYPE)
> +    cond_code = VEC_COND_EXPR;
> +
>    /* This transformation is only worthwhile if we don't have to wrap ARG
>       in a SAVE_EXPR and the operation can be simplified on at least one
>       of the branches once its pushed inside the COND_EXPR.  */
>    if (!TREE_CONSTANT (arg)
>        && (TREE_SIDE_EFFECTS (arg)
>           || TREE_CONSTANT (true_value) || TREE_CONSTANT (false_value)))
>      return NULL_TREE;
>
>    arg = fold_convert_loc (loc, arg_type, arg);
>    if (lhs == 0)
> @@ -6004,21 +6009,21 @@ fold_binary_op_with_conditional_arg (loc
>        if (cond_first_p)
>         rhs = fold_build2_loc (loc, code, type, false_value, arg);
>        else
>         rhs = fold_build2_loc (loc, code, type, arg, false_value);
>      }
>
>    /* Check that we have simplified at least one of the branches.  */
>    if (!TREE_CONSTANT (arg) && !TREE_CONSTANT (lhs) && !TREE_CONSTANT (rhs))
>      return NULL_TREE;
>
> -  return fold_build3_loc (loc, COND_EXPR, type, test, lhs, rhs);
> +  return fold_build3_loc (loc, cond_code, type, test, lhs, rhs);
>  }
>
>
>  /* Subroutine of fold() that checks for the addition of +/- 0.0.
>
>     If !NEGATE, return true if ADDEND is +/-0.0 and, for all X of type
>     TYPE, X + ADDEND is the same as X.  If NEGATE, return true if X -
>     ADDEND is the same as X.
>
>     X + 0 and X - 0 both give X when X is NaN, infinite, or nonzero
> @@ -9864,30 +9869,34 @@ fold_binary_loc (location_t loc,
>        if (TREE_CODE (arg1) == COMPOUND_EXPR
>           && reorder_operands_p (arg0, TREE_OPERAND (arg1, 0)))
>         {
>           tem = fold_build2_loc (loc, code, type, op0,
>                              fold_convert_loc (loc, TREE_TYPE (op1),
>                                                TREE_OPERAND (arg1, 1)));
>           return build2_loc (loc, COMPOUND_EXPR, type, TREE_OPERAND (arg1,
> 0),
>                              tem);
>         }
>
> -      if (TREE_CODE (arg0) == COND_EXPR || COMPARISON_CLASS_P (arg0))
> +      if (TREE_CODE (arg0) == COND_EXPR
> +         || TREE_CODE (arg0) == VEC_COND_EXPR
> +         || COMPARISON_CLASS_P (arg0))
>         {
>           tem = fold_binary_op_with_conditional_arg (loc, code, type, op0,
> op1,
>                                                      arg0, arg1,
>                                                      /*cond_first_p=*/1);
>           if (tem != NULL_TREE)
>             return tem;
>         }
>
> -      if (TREE_CODE (arg1) == COND_EXPR || COMPARISON_CLASS_P (arg1))
> +      if (TREE_CODE (arg1) == COND_EXPR
> +         || TREE_CODE (arg1) == VEC_COND_EXPR
> +         || COMPARISON_CLASS_P (arg1))
>         {
>           tem = fold_binary_op_with_conditional_arg (loc, code, type, op0,
> op1,
>                                                      arg1, arg0,
>                                                      /*cond_first_p=*/0);
>           if (tem != NULL_TREE)
>             return tem;
>         }
>      }
>
>    switch (code)
>

On Tue, Oct 30, 2012 at 01:14:40PM +0100, Marc Glisse wrote:
> On Tue, 30 Oct 2012, Marek Polacek wrote:
> 
> >On Tue, Oct 30, 2012 at 08:05:13AM +0100, Marc Glisse wrote:
> >>Hello,
> >>
> >>one more optimization that needed help for vectors, it crashed on
> >>(x<y)<0. Because of PR 55001, testcases are awkward to add (I could
> >>do a x86-only one if needed).
> >>
> >>bootstrap+testsuite.
> >>
> >>2012-10-30  Marc Glisse  <marc.glisse@inria.fr>
> >>
> >>	* fold-const.c (fold_binary_op_with_conditional_arg): Handle vectors.
> >>	(fold_binary_loc): call it for VEC_COND_EXPR.

Capital C instead of lowercase.

> >
> >Patch missing?
> 
> Indeed, thanks for the notice.

This regressed
FAIL: g++.dg/other/vector-compare.C (internal compiler error)
FAIL: g++.dg/other/vector-compare.C (test for excess errors)
on i686-linux.  The problem is that tree-vect-generic.c doesn't expand
BLKmode VEC_COND_EXPR piecewise.  So, either fold-const on this optimization
needs to test whether expand_vec_cond_expr_p is true for the chosen
pair of comparison type and operand type, or tree-vect-generic.c needs to be
tought to expand VEC_COND_EXPR piecewise if expand_vec_cond_expr_p is not
true for the particular VEC_COND_EXPR.

	Jakub

On Wed, Oct 31, 2012 at 9:24 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Oct 30, 2012 at 01:14:40PM +0100, Marc Glisse wrote:
>> On Tue, 30 Oct 2012, Marek Polacek wrote:
>>
>> >On Tue, Oct 30, 2012 at 08:05:13AM +0100, Marc Glisse wrote:
>> >>Hello,
>> >>
>> >>one more optimization that needed help for vectors, it crashed on
>> >>(x<y)<0. Because of PR 55001, testcases are awkward to add (I could
>> >>do a x86-only one if needed).
>> >>
>> >>bootstrap+testsuite.
>> >>
>> >>2012-10-30  Marc Glisse  <marc.glisse@inria.fr>
>> >>
>> >>    * fold-const.c (fold_binary_op_with_conditional_arg): Handle vectors.
>> >>    (fold_binary_loc): call it for VEC_COND_EXPR.
>
> Capital C instead of lowercase.
>
>> >
>> >Patch missing?
>>
>> Indeed, thanks for the notice.
>
> This regressed
> FAIL: g++.dg/other/vector-compare.C (internal compiler error)
> FAIL: g++.dg/other/vector-compare.C (test for excess errors)
> on i686-linux.  The problem is that tree-vect-generic.c doesn't expand
> BLKmode VEC_COND_EXPR piecewise.  So, either fold-const on this optimization
> needs to test whether expand_vec_cond_expr_p is true for the chosen
> pair of comparison type and operand type, or tree-vect-generic.c needs to be
> tought to expand VEC_COND_EXPR piecewise if expand_vec_cond_expr_p is not
> true for the particular VEC_COND_EXPR.

The latter (tree-vect-generic.c needs to expand it).

Richard.

>         Jakub

On Wed, 31 Oct 2012, Jakub Jelinek wrote:

> On Tue, Oct 30, 2012 at 01:14:40PM +0100, Marc Glisse wrote:
>> On Tue, 30 Oct 2012, Marek Polacek wrote:
>>
>>> On Tue, Oct 30, 2012 at 08:05:13AM +0100, Marc Glisse wrote:
>>>> Hello,
>>>>
>>>> one more optimization that needed help for vectors, it crashed on
>>>> (x<y)<0. Because of PR 55001, testcases are awkward to add (I could
>>>> do a x86-only one if needed).
>>>>
>>>> bootstrap+testsuite.
>>>>
>>>> 2012-10-30  Marc Glisse  <marc.glisse@inria.fr>
>>>>
>>>> 	* fold-const.c (fold_binary_op_with_conditional_arg): Handle vectors.
>>>> 	(fold_binary_loc): call it for VEC_COND_EXPR.
>
> Capital C instead of lowercase.

Ok.

>>> Patch missing?
>>
>> Indeed, thanks for the notice.
>
> This regressed
> FAIL: g++.dg/other/vector-compare.C (internal compiler error)
> FAIL: g++.dg/other/vector-compare.C (test for excess errors)
> on i686-linux.

Thanks. I am surprised, I expected this problem to happen only for code 
where the optimization was previously crashing. Actually, before my patch, 
the code was apparently silently miscompiled. We appear to be missing code 
that would check that the result of a vector operation is a vector (in 
particular not a boolean).

> The problem is that tree-vect-generic.c doesn't expand
> BLKmode VEC_COND_EXPR piecewise.  So, either fold-const on this optimization
> needs to test whether expand_vec_cond_expr_p is true for the chosen
> pair of comparison type and operand type, or tree-vect-generic.c needs to be
> tought to expand VEC_COND_EXPR piecewise if expand_vec_cond_expr_p is not
> true for the particular VEC_COND_EXPR.

The second one is needed anyway (that's PR 55001, which I mention in my 
email). One quick hack as long as it isn't done would be to replace:
     cond_code = VEC_COND_EXPR;
with
     return NULL_TREE;
(and a comment linking to PR55001 and this conversation).

Now I am a bit afraid that the vector lowering pass is not the last one, 
and some later pass might call fold and re-create unexpandable vector 
operations, in which case the first option might also be needed... but I 
hope not.