Re: [vec-cmp, patch 1/6] Add optabs for vector comparison

[Jeff Law <law@redhat.com>]

On 10/22/2015 10:12 AM, Ilya Enkovich wrote:
> 2015-10-22 18:52 GMT+03:00 Jeff Law <law@redhat.com>:
>> On 10/22/2015 04:35 AM, Ilya Enkovich wrote:
>>>
>>> 2015-10-21 20:25 GMT+03:00 Jeff Law <law@redhat.com>:
>>>>
>>>> On 10/08/2015 08:52 AM, Ilya Enkovich wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> This series introduces autogeneration of vector comparison and its
>>>>> support
>>>>> on i386 target.  It lets comparison statements to be vectorized into
>>>>> vector
>>>>> comparison instead of VEC_COND_EXPR.  This allows to avoid some
>>>>> restrictions
>>>>> implied by boolean patterns.  This series applies on top of bolean
>>>>> vectors
>>>>> series [1].
>>>>>
>>>>> This patch introduces optabs for vector comparison.
>>>>>
>>>>> [1] https://gcc.gnu.org/ml/gcc-patches/2015-10/msg00215.html
>>>>>
>>>>> Thanks,
>>>>> Ilya
>>>>> --
>>>>> gcc/
>>>>>
>>>>> 2015-10-08  Ilya Enkovich  <enkovich.gnu@gmail.com>
>>>>>
>>>>>           * expr.c (do_store_flag): Use expand_vec_cmp_expr for mask
>>>>> results.
>>>>>           * optabs-query.h (get_vec_cmp_icode): New.
>>>>>           * optabs-tree.c (expand_vec_cmp_expr_p): New.
>>>>>           * optabs-tree.h (expand_vec_cmp_expr_p): New.
>>>>>           * optabs.c (vector_compare_rtx): Add OPNO arg.
>>>>>           (expand_vec_cond_expr): Adjust to vector_compare_rtx change.
>>>>>           (expand_vec_cmp_expr): New.
>>>>>           * optabs.def (vec_cmp_optab): New.
>>>>>           (vec_cmpu_optab): New.
>>>>>           * optabs.h (expand_vec_cmp_expr): New.
>>>>>           * tree-vect-generic.c (expand_vector_comparison): Add vector
>>>>>           comparison optabs check.
>>>>>
>>>>>
>>>>> diff --git a/gcc/optabs-tree.c b/gcc/optabs-tree.c
>>>>> index 3b03338..aa863cf 100644
>>>>> --- a/gcc/optabs-tree.c
>>>>> +++ b/gcc/optabs-tree.c
>>>>> @@ -320,6 +320,19 @@ supportable_convert_operation (enum tree_code code,
>>>>>       return false;
>>>>>     }
>>>>>
>>>>> +/* Return TRUE if appropriate vector insn is available
>>>>> +   for vector comparison expr with vector type VALUE_TYPE
>>>>> +   and resulting mask with MASK_TYPE.  */
>>>>> +
>>>>> +bool
>>>>> +expand_vec_cmp_expr_p (tree value_type, tree mask_type)
>>>>> +{
>>>>> +  enum insn_code icode = get_vec_cmp_icode (TYPE_MODE (value_type),
>>>>> +                                           TYPE_MODE (mask_type),
>>>>> +                                           TYPE_UNSIGNED (value_type));
>>>>> +  return (icode != CODE_FOR_nothing);
>>>>> +}
>>>>> +
>>>>
>>>>
>>>> Nothing inherently wrong with the code, but it seems like it's in the
>>>> wrong
>>>> place.  Why optabs-tree rather than optabs-query?
>>>
>>>
>>> Because it uses tree type for arguments. There is no single tree usage
>>> in optabs-query.c. I think expand_vec_cond_expr_p is in optabs-tree
>>> for the same reason.
>>
>> Note that expand_vec_cond_expr_p doesn't rely on enum insn code.  Well, it
>> relies on enum insn code being an integer and CODE_FOR_nothing always having
>> the value zero, which is probably worse.
>
> Actually it also uses CODE_FOR_nothing in comparison:
>
>        || get_vcond_icode (TYPE_MODE (value_type), TYPE_MODE (cmp_op_type),
>                            TYPE_UNSIGNED (cmp_op_type)) == CODE_FOR_nothing)
>
> There are also two other instances of CODE_FOR_nothing in
> optabs-tree.c. Do you want to get rid of "#include insn-codes.h" in
> optabs-tree.c? Will it be really better if we replace it with
> "#include optabs-query.h"?
Sigh.  I searched for the enum type, not for CODE_FOR_nothing ;(  My bad.

If it's easy to get rid of, yes.  I believe we've got 3 uses of 
CODE_FOR_nothing.  AFAICT in none of those cases do we care about the 
code other than does it correspond to CODE_FOR_nothing.

Ideally we'd like to have both optabs-query and optabs-tree not know 
about insn codes.  The former is supposed to be IR agnostic, but insn 
codes are part of the RTL IR, so that's a wart.  The latter is supposed 
to be tree specific and thus shouldn't know about the RTL IR either.

I'd settle for getting the wart out of optabs-tree and we can put 
further cleanup of optabs-query in the queue.

To get the wart out of optabs-tree all I think we need is a true boolean 
function that tells us if there's a suitable optab.

It's unfortunate that the routines exported by optabs-query are 
can_{extend,float,fix}_p since those would fairly natural for the 
boolean query we want to make and they're used elsewhere, but not in a 
boolean form.

I think that we ought to rename the existing uses & definition of 
can_XXX_p that are exported by optabs-query.c, then creating new 
can_XXX_p for those uses that just care about the boolean status should 
work.  At that point we remove insn-codes.h from optab-tree.c.

Jeff