On Tue, Aug 23, 2011 at 12:24 PM, Artem Shinkarov
<artyom.shinkar...@gmail.com> wrote:
> On Tue, Aug 23, 2011 at 11:08 AM, Richard Guenther
> <richard.guent...@gmail.com> wrote:
>> On Tue, Aug 23, 2011 at 11:44 AM, Artem Shinkarov
>> <artyom.shinkar...@gmail.com> wrote:
>>> On Tue, Aug 23, 2011 at 9:17 AM, Richard Guenther
>>> <richard.guent...@gmail.com> wrote:
>>>> On Mon, Aug 22, 2011 at 11:11 PM, Artem Shinkarov
>>>> <artyom.shinkar...@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.
