[PATCH] tree-optimization/98211 - fix bogus vectorization of conversion

[Richard Biener]

On Fri, 11 Dec 2020, Richard Sandiford wrote:

> Richard Biener <rguenther@suse.de> writes:
> > On Fri, 11 Dec 2020, Richard Sandiford wrote:
> >> > diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
> >> > index a4980a931a9..d3ab8aa1c29 100644
> >> > --- a/gcc/tree-vect-stmts.c
> >> > +++ b/gcc/tree-vect-stmts.c
> >> > @@ -5123,6 +5123,17 @@ vectorizable_assignment (vec_info *vinfo,
> >> >  		       GET_MODE_SIZE (TYPE_MODE (vectype_in)))))
> >> >      return false;
> >> >  
> >> > +  if (VECTOR_BOOLEAN_TYPE_P (vectype)
> >> > +      && !VECTOR_BOOLEAN_TYPE_P (vectype_in))
> >> > +    {
> >> > +      if (dump_enabled_p ())
> >> > +	dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> >> > +			 "can't convert between boolean and non "
> >> > +			 "boolean vectors %T\n", TREE_TYPE (op));
> >> > +
> >> > +      return false;
> >> > +    }
> >> 
> >> What do you think about my comment in the PR, about instead checking for:
> >> 
> >>   VECTOR_BOOLEAN_TYPE_P (vectype)
> >>   != VECTOR_BOOLEAN_TYPE_P (vectype_in)
> >> 
> >> ?  I'm not sure vectorizable_assignment can handle converting a vector
> >> boolean type to a non-vector boolean type either, and checking for both
> >> directions seems to match the dump message more closely.
> >
> > The condition matches that used in vectorizable_conversion, I'm not sure
> > whether/why we allow the reverse but then if the precisions match
> > and we do want to use the bool vector as "data" then why should a
> > conversion fail?  The condition is specifically to guard a missing
> > "sign extension" which should be done via patterns and not conversions.
> 
> I think vectorizable_conversion should be able to handle the reverse case.
> I'm just not sure vectorizable_assignment (with its VCE) necessarily can.

But what's the semantic of vector bool -> vector int?  For the other
direction we say pattern recog has to make it defined by using
a ?:, but I don't think pattern recog does anything for the reverse,
there the source 'bool' is simply treated as char with 0 or 1 value.
So the vectorizer itself would need to emit the vector bool to
vector conversion which I think it does not.

> For vector bool -> vector int, are the upper bits of the vector bool
> already supposed to be copies of the low bit (or perhaps instead supposed
> to be zero)?  I'm not sure we guarantee that for SVE.  Only the low bit
> of a vector bool element is really significant there.
> 
> And if the upper bits of the bool elements are sign copies, is that
> necessarily what we want for bool -> int?  For the C semantics I'd
> have expected the result should be 0 or 1 instead.

Not sure - I'd like to see an actual testcase where we end up
with a conversion in this direction.  I think it simply can't
happen?  Maybe we're clever and with traditional bool vectors
(non-mask) we can vectorize

  a[i] = a[i] != 0 ? -1 : 0;

as

  vector<bool:N> tem = a[i] != 0;
  vector<int> tem2 = V_C_E (tem);
  a[i] = tem2;

?

> > I've misinterpreted your comment to refer to the existing odd
> > allowance in the test following this:
> >
> >   /* We do not handle bit-precision changes.  */
> >   if ((CONVERT_EXPR_CODE_P (code)
> >        || code == VIEW_CONVERT_EXPR)
> >       && INTEGRAL_TYPE_P (TREE_TYPE (scalar_dest))
> >       && (!type_has_mode_precision_p (TREE_TYPE (scalar_dest))
> >           || !type_has_mode_precision_p (TREE_TYPE (op)))
> >       /* But a conversion that does not change the bit-pattern is ok.  */
> >       && !((TYPE_PRECISION (TREE_TYPE (scalar_dest))
> >             > TYPE_PRECISION (TREE_TYPE (op)))
> >            && TYPE_UNSIGNED (TREE_TYPE (op)))
> >       /* Conversion between boolean types of different sizes is
> >          a simple assignment in case their vectypes are same
> >          boolean vectors.  */
> >       && (!VECTOR_BOOLEAN_TYPE_P (vectype)
> >           || !VECTOR_BOOLEAN_TYPE_P (vectype_in)))
> > ^^^
> >
> > which I have since removed (and where I also shortly thought that
> > a VECTOR_BOOLEAN_TYPE_P == VECTOR_BOOLEAN_TYPE_P was intended)
> 
> Yeah, initially I was thinking that changing that to != would be
> enough to fix the bug.  But it feels like the condition ought to be
> symmetric wherever we put it.
> 
> Thanks,
> Richard