Re: VEC_COND_EXPR optimizations

[Marc Glisse <marc.glisse@inria.fr>]

On Fri, 31 Jul 2020, Richard Biener wrote:

> On Fri, Jul 31, 2020 at 1:39 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
>>
>> On Fri, Jul 31, 2020 at 1:35 PM Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>>
>>> On Thu, Jul 30, 2020 at 9:49 AM Marc Glisse <marc.glisse@inria.fr> wrote:
>>>>
>>>> When vector comparisons were forced to use vec_cond_expr, we lost a number
>>>> of optimizations (my fault for not adding enough testcases to prevent
>>>> that). This patch tries to unwrap vec_cond_expr a bit so some
>>>> optimizations can still happen.
>>>>
>>>> I wasn't planning to add all those transformations together, but adding
>>>> one caused a regression, whose fix introduced a second regression, etc.
>>>>
>>>> Using a simple fold_binary internally looks like an ok compromise to me.
>>>> It remains cheap enough (not recursive, and vector instructions are not
>>>> that frequent), while still allowing more than const_binop (X|0 or X&X for
>>>> instance). The transformations are quite conservative with :s and folding
>>>> only if everything simplifies, we may want to relax this later. And of
>>>> course we are going to miss things like a?b:c + a?c:b -> b+c.
>>>>
>>>> In terms of number of operations, some transformations turning 2
>>>> VEC_COND_EXPR into VEC_COND_EXPR + BIT_IOR_EXPR + BIT_NOT_EXPR might not
>>>> look like a gain... I expect the bit_not disappears in most cases, and
>>>> VEC_COND_EXPR looks more costly than a simpler BIT_IOR_EXPR.
>>>>
>>>> I am a bit confused that with avx512 we get types like "vector(4)
>>>> <signed-boolean:2>" with :2 and not :1 (is it a hack so true is 1 and not
>>>> -1?), but that doesn't matter for this patch.
>>>>
>>>> Regtest+bootstrap on x86_64-pc-linux-gnu
>>>
>>> +  (with
>>> +   {
>>> +     tree rhs1, rhs2 = NULL;
>>> +     rhs1 = fold_binary (op, type, @1, @3);
>>> +     if (rhs1 && is_gimple_val (rhs1))
>>> +       rhs2 = fold_binary (op, type, @2, @3);
>>>
>>> ICK.  I guess a more match-and-simplify way would be

I was focused on avoiding recursion, but indeed that's independent, I 
could have set a trivial valueize function for that.

>>>    (with
>>>     {
>>>       tree rhs1, rhs2;
>>>       gimple_match_op op (gimple_match_cond::UNCOND, op,
>>>                                       type, @1, @3);
>>>       if (op.resimplify (NULL, valueize)

Oh, so you would be ok with something that recurses without limit? I 
thought we were avoiding this because it may allow some compile time 
explosion with pathological examples.

>>>           && gimple_simplified_result_is_gimple_val (op))
>>>        {
>>>          rhs1 = op.ops[0];
>>>          ... other operand ...
>>>        }
>>>
>>> now in theory we could invent some new syntax for this, like
>>>
>>>  (simplify
>>>   (op (vec_cond:s @0 @1 @2) @3)
>>>   (vec_cond @0 (op:x @1 @3) (op:x @2 @3)))
>>>
>>> and pick something better instead of :x (:s is taken,
>>> would be 'simplified', :c is taken would be 'constexpr', ...).
>>>
>>> _Maybe_ just
>>>
>>>  (simplify
>>>   (op (vec_cond:s @0 @1 @2) @3)
>>>   (vec_cond:x @0 (op @1 @3) (op @2 @3)))
>>>
>>> which would have the same practical meaning as passing
>>> NULL for the seq argument to simplification - do not allow
>>> any intermediate stmt to be generated.
>>
>> Note I specifically do not like those if (it-simplifies) checks
>> because we already would code-generate those anyway.  For
>>
>> (simplify
>>   (plus (vec_cond:s @0 @1 @2) @3)
>>   (vec_cond @0 (plus @1 @3) (plus @2 @3)))
>>
>> we get
>>
>>                     res_op->set_op (VEC_COND_EXPR, type, 3);
>>                     res_op->ops[0] = captures[1];
>>                     res_op->ops[0] = unshare_expr (res_op->ops[0]);
>>                     {
>>                       tree _o1[2], _r1;
>>                       _o1[0] = captures[2];
>>                       _o1[1] = captures[4];
>>                       gimple_match_op tem_op (res_op->cond.any_else
>> (), PLUS_EXPR, TREE_TYPE (_o1[0]), _o1[0], _o1[1]);
>>                       tem_op.resimplify (lseq, valueize);
>>                       _r1 = maybe_push_res_to_seq (&tem_op, lseq);  (****)
>>                       if (!_r1) return false;
>>                       res_op->ops[1] = _r1;
>>                     }
>>                     {
>>                       tree _o1[2], _r1;
>>                       _o1[0] = captures[3];
>>                       _o1[1] = captures[4];
>>                       gimple_match_op tem_op (res_op->cond.any_else
>> (), PLUS_EXPR, TREE_TYPE (_o1[0]), _o1[0], _o1[1]);
>>                       tem_op.resimplify (lseq, valueize);
>>                       _r1 = maybe_push_res_to_seq (&tem_op, lseq);  (***)
>>                       if (!_r1) return false;
>>                       res_op->ops[2] = _r1;
>>                     }
>>                     res_op->resimplify (lseq, valueize);
>>                     return true;
>>
>> and the only change required would be to pass NULL to maybe_push_res_to_seq
>> here instead of lseq at the (***) marked points.
>
> (simplify
>  (plus (vec_cond:s @0 @1 @2) @3)
>  (vec_cond:l @0 (plus @1 @3) (plus @2 @3)))
>
> 'l' for 'force leaf'.  I'll see if I can quickly cme up with a patch.

Does it have a clear meaning for GENERIC? I guess that's probably not such 
a big problem.

There are a number of transformations that we would like to perform "if 
<something> simplifies", but I don't know if it will always have exactly 
this form of "if this part of the output pattern doesn't simplify, give 
up". In some cases, we might prefer "ok if at least one of the branches 
simplifies" for instance. Or "ok if this generates at most one extra 
statement". Even for this particular transformation, I am not sure this is 
the right condition.

Your suggestion looks good (although I understand better the version with 
a mark on plus instead of vec_cond_expr), I just want to avoid wasting 
your time if it turns out we don't use it that much in the end (I have no 
idea yet).

-- 
Marc Glisse