On Fri, 11 Dec 2020, Richard Sandiford wrote:
> Richard Biener <rguent...@suse.de> writes:
> > Pattern recog incompletely handles some bool cases but we shouldn't
> > miscompile as a result but not vectorize. Unfortunately
> > vectorizable_assignment lets invalid conversions (that
> > vectorizable_conversion rejects) slip through. The following
> > rectifies that.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> >
> > 2020-12-10 Richard Biener <rguent...@suse.de>
> >
> > PR tree-optimization/98211
> > * tree-vect-stmts.c (vectorizable_assignment): Disallow
> > invalid conversions to bool vector types.
> >
> > * gcc.dg/pr98211.c: New testcase.
> > ---
> > gcc/testsuite/gcc.dg/pr98211.c | 51 ++++++++++++++++++++++++++++++++++
> > gcc/tree-vect-stmts.c | 11 ++++++++
> > 2 files changed, 62 insertions(+)
> > create mode 100644 gcc/testsuite/gcc.dg/pr98211.c
> >
> > diff --git a/gcc/testsuite/gcc.dg/pr98211.c b/gcc/testsuite/gcc.dg/pr98211.c
> > new file mode 100644
> > index 00000000000..cea371dcee7
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/pr98211.c
> > @@ -0,0 +1,51 @@
> > +/* { dg-do run } */
> > +/* { dg-options "-std=gnu90 -O3 -fgimple" } */
> > +
> > +int test_var_3;
> > +short arr_20[16];
> > +void __GIMPLE (ssa,startwith("slp"))
> > +test (int var_1, short int a, short int b, short int c, short int d)
> > +{
> > + _Bool tem2;
> > + _Bool tem;
> > + unsigned int i_5;
> > + int _24;
> > + _Bool _28;
> > + short int _30;
> > + short int _32;
> > +
> > + __BB(2):
> > + _24 = test_var_3;
> > + tem_25 = _24 != 0;
> > + tem2_26 = var_1_11(D) != 0;
> > + _28 = tem_25 | tem2_26;
> > + _30 = _28 != _Literal (_Bool) 0 ? a_16(D) : b_15(D);
> > + arr_20[0u] = _30;
> > + _32 = _28 != _Literal (_Bool) 0 ? c_19(D) : d_18(D);
> > + arr_20[8u] = _32;
> > + arr_20[1u] = _30;
> > + arr_20[9u] = _32;
> > + arr_20[2u] = _30;
> > + arr_20[10u] = _32;
> > + arr_20[3u] = _30;
> > + arr_20[11u] = _32;
> > + arr_20[4u] = _30;
> > + arr_20[12u] = _32;
> > + arr_20[5u] = _30;
> > + arr_20[13u] = _32;
> > + arr_20[6u] = _30;
> > + arr_20[14u] = _32;
> > + arr_20[7u] = _30;
> > + arr_20[15u] = _32;
> > + return;
> > +}
> > +
> > +
> > +int
> > +main()
> > +{
> > + test (1, 0x88, 0x77, 0x77, 0x88);
> > + if (arr_20[0] != 0x88)
> > + __builtin_abort ();
> > + return 0;
> > +}
> > 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'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)
Richard.
