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

[Jeff Law <law at redhat dot 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.

We should clean both of these up so that:

  1. We don't need enum insn_code in optabs-tree
  2. We don't implicitly rely on CODE_FOR_nothing == 0

It may be as simple as a adding a predicate function to optabs-query 
that returns true/false if there's a suitable icode, then using that 
predicate in optabs-tree.

jeff