Re: Vector Comparison patch

[Artem Shinkarov <artyom dot shinkaroff at gmail dot com>]

On Tue, Aug 23, 2011 at 11:33 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Tue, Aug 23, 2011 at 12:24 PM, Artem Shinkarov
> <artyom.shinkaroff@gmail.com> wrote:
>> On Tue, Aug 23, 2011 at 11:08 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Tue, Aug 23, 2011 at 11:44 AM, Artem Shinkarov
>>> <artyom.shinkaroff@gmail.com> wrote:
>>>> On Tue, Aug 23, 2011 at 9:17 AM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Mon, Aug 22, 2011 at 11:11 PM, Artem Shinkarov
>>>>> <artyom.shinkaroff@gmail.com> wrote:
>>>>>> I'll just send you my current version. I'll be a little bit more specific.
>>>>>>
>>>>>> The problem starts when you try to lower the following expression:
>>>>>>
>>>>>> x = a > b;
>>>>>> x1 = vcond <x != 0, -1, 0>
>>>>>> vcond <x1, c, d>
>>>>>>
>>>>>> Now, you go from the beginning to the end of the block, and you cannot
>>>>>> leave a > b, because only vconds are valid expressions to expand.
>>>>>>
>>>>>> Now, you meet a > b first. You try to transform it into vcond <a > b,
>>>>>> -1, 0>, you build this expression, then you try to gimplify it, and
>>>>>> you see that you have something like:
>>>>>>
>>>>>> x' = a >b;
>>>>>> x = vcond <x', -1, 0>
>>>>>> x1 = vcond <x != 0, -1, 0>
>>>>>> vcond <x1, c, d>
>>>>>>
>>>>>> and your gsi stands at the x1 now, so the gimplification created a
>>>>>> comparison that optab would not understand. And I am not really sure
>>>>>> that you would be able to solve this problem easily.
>>>>>>
>>>>>> It would helpr, if you could create vcond<x, op, y, op0, op1>, but you
>>>>>> cant and x op y is a single tree that must be gimplified, and I am not
>>>>>> sure that you can persuade gimplifier to leave this expression
>>>>>> untouched.
>>>>>>
>>>>>> In the attachment the current version of the patch.
>>>>>
>>>>> I can't reproduce it with your patch. ?For
>>>>>
>>>>> #define vector(elcount, type) ?\
>>>>> ? ?__attribute__((vector_size((elcount)*sizeof(type)))) type
>>>>>
>>>>> vector (4, float) x, y;
>>>>> vector (4, int) a,b;
>>>>> int
>>>>> main (int argc, char *argv[])
>>>>> {
>>>>> ?vector (4, int) i0 = x < y;
>>>>> ?vector (4, int) i1 = i0 ? a : b;
>>>>> ?return 0;
>>>>> }
>>>>>
>>>>> I get from the C frontend:
>>>>>
>>>>> ?vector(4) int i0 = ?VEC_COND_EXPR < SAVE_EXPR <x < y> , { -1, -1,
>>>>> -1, -1 } , { 0, 0, 0, 0 } > ;
>>>>> ?vector(4) int i1 = ?VEC_COND_EXPR < SAVE_EXPR <i0> , SAVE_EXPR <a> ,
>>>>> SAVE_EXPR <b> > ;
>>>>>
>>>>> but I have expected i0 != 0 in the second VEC_COND_EXPR.
>>>>
>>>> I don't put it there. This patch adds != 0, rather removing. But this
>>>> could be changed.
>>>
>>> ?
>>>
>>>>> I do see that the gimplifier pulls away the condition for the first
>>>>> VEC_COND_EXPR though:
>>>>>
>>>>> ?x.0 = x;
>>>>> ?y.1 = y;
>>>>> ?D.2735 = x.0 < y.1;
>>>>> ?D.2734 = D.2735;
>>>>> ?D.2736 = D.2734;
>>>>> ?i0 = [vec_cond_expr] ?VEC_COND_EXPR < D.2736 , { -1, -1, -1, -1 } ,
>>>>> { 0, 0, 0, 0 } > ;
>>>>>
>>>>> which is, I believe because of the SAVE_EXPR wrapped around the
>>>>> comparison. ?Why do you bother wrapping all operands in save-exprs?
>>>>
>>>> I bother because they could be MAYBE_CONST which breaks the
>>>> gimplifier. But I don't really know if you can do it better. I can
>>>> always do this checking on operands of constructed vcond...
>>>
>>> Err, the patch does
>>>
>>> + ?/* Avoid C_MAYBE_CONST in VEC_COND_EXPR. ?*/
>>> + ?tmp = c_fully_fold (ifexp, false, &maybe_const);
>>> + ?ifexp = save_expr (tmp);
>>> + ?wrap &= maybe_const;
>>>
>>> why is
>>>
>>> ?ifexp = save_expr (tmp);
>>>
>>> necessary here? ?SAVE_EXPR is if you need to protect side-effects
>>> from being evaluated twice if you use an operand twice. ?But all
>>> operands are just used a single time.
>>
>> Again, the only reason why save_expr is there is to avoid MAYBE_CONST
>> nodes to break the gimplification. But may be it is a wrong way of
>> doing it, but it does the job.
>>
>>> And I expected, instead of
>>>
>>> + ?if ((COMPARISON_CLASS_P (ifexp)
>>> + ? ? ? && TREE_TYPE (TREE_OPERAND (ifexp, 0)) != TREE_TYPE (op1))
>>> + ? ? ?|| TREE_TYPE (ifexp) != TREE_TYPE (op1))
>>> + ? ?{
>>> + ? ? ?tree comp_type = COMPARISON_CLASS_P (ifexp)
>>> + ? ? ? ? ? ? ? ? ? ? ?? TREE_TYPE (TREE_OPERAND (ifexp, 0))
>>> + ? ? ? ? ? ? ? ? ? ? ?: TREE_TYPE (ifexp);
>>> +
>>> + ? ? ?op1 = convert (comp_type, op1);
>>> + ? ? ?op2 = convert (comp_type, op2);
>>> + ? ? ?vcond = build3 (VEC_COND_EXPR, comp_type, ifexp, op1, op2);
>>> + ? ? ?vcond = convert (TREE_TYPE (op1), vcond);
>>> + ? ?}
>>> + ?else
>>> + ? ?vcond = build3 (VEC_COND_EXPR, TREE_TYPE (op1), ifexp, op1, op2);
>>>
>>> ?if (!COMPARISON_CLASS_P (ifexp))
>>> ? ?ifexp = build2 (NE_EXPR, TREE_TYPE (ifexp), ifexp,
>>> ? ? ? ? ? ? ? ? ? ? ? ? build_vector_from_val (TREE_TYPE (ifexp), 0));
>>>
>>> ?if (TREE_TYPE (ifexp) != TREE_TYPE (op1))
>>> ? ?{
>>> ...
>>>
>> Why?
>> This is a function to constuct any vcond. The result of ifexp is
>> always signed integer vector if it is a comparison, but we need to
>> make sure that all the elements of vcond have the same type.
>>
>> And I didn't really understand if we can guarantee that vector
>> comparison would not be lifted out by the gimplifier. It happens in
>> case I put this save_expr, it could possibly happen in some other
>> cases. How can we prevent that?
>
> We don't need to prevent it. ?If the C frontend makes sure that the
> mask of a VEC_COND_EXPR is always {-1,...} or {0,....} by expanding
> mask ? v1 : v2 to VEC_COND_EXPR <mask != 0, v1, v2> then
> the expansion can do the obvious thing with a non-comparison mask
> (have another md pattern for this case to handle AMD XOP vcond
> or simply emit bitwise mask operations).
>
> The gimplifier shouldn't unnecessarily pull out the comparison, but
> you instructed it to - by means of wrapping it inside a SAVE_EXPR.
>
> Richard.
>

I'm confused.
There is a set of problems which are tightly connected and you address
only one one of them.

I need to do something with C_MAYBE_CONST_EXPR node to allow the
gimplification of the expression. In order to achieve that I am
wrapping expression which can contain C_MAYBE_EXPR_NODE into
SAVE_EXPR. This works fine, but, the vector condition is lifted out.
So the question is how to get rid of C_MAYBE_CONST_EXPR nodes, making
sure that the expression is still inside VEC_COND_EXPR?

All the rest is fine -- a > b is transformed to VEC_COND_EXPR of the
integer type, and when we are using it we can add != 0 to the mask, no
problem. The problem is to make sure that the vector expression is not
lifted out from the VEC_COND_EXPR and that C_MAYBE_CONST_EXPRs are
also no there at the same time.

Artem.