diff mbox

Fix bool vs. unsigned:1 vectorization (PR tree-optimization/79284)

Message ID 20170131182637.GI14051@tucnak
State New
Headers show

Commit Message

Jakub Jelinek Jan. 31, 2017, 6:26 p.m. UTC 
Hi!

useless_type_conversion_p says that precision 1 unsigned BOOLEAN_TYPE
conversion to/from precision 1 unsigned integral type is useless,
but apparently the vectorizer heavily relies on the BOOLEAN_TYPE vs.
unsigned integral:1 distinction (uses VECTOR_BOOLEAN_TYPE_P types
only for real BOOLEAN_TYPE).  As the conversion is useless, we can end up
as in the following testcase with e.g. binary operations which have boolean
on one side and unsigned int:1 on another.

The following patch replaces all explicit BOOLEAN_TYPE checks in the
vectorizer with a new macro that handles unsigned integral:1 the same
as BOOLEAN_TYPE.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2017-01-31  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/79284
	* tree.h (INTEGRAL_BOOLEAN_TYPE_P): Define.
	* tree-vect-stmts.c (vect_get_vec_def_for_operand,
	vectorizable_mask_load_store, vectorizable_operation,
	vect_is_simple_cond, get_same_sized_vectype): Use it instead
	of comparing TREE_CODE of a type against BOOLEAN_TYPE.
	* tree-vect-patterns.c (check_bool_pattern, search_type_for_mask_1,
	vect_recog_bool_pattern, vect_recog_mask_conversion_pattern): Likewise.
	* tree-vect-slp.c (vect_get_constant_vectors): Likewise.
	* tree-vect-loop.c (vect_determine_vectorization_factor): Likewise.
testsuite/
	* gcc.c-torture/compile/pr79284.c: New test.


	Jakub

Comments

Jeff Law Jan. 31, 2017, 10:41 p.m. UTC | #1 
On 01/31/2017 11:26 AM, Jakub Jelinek wrote:
> Hi!
>
> useless_type_conversion_p says that precision 1 unsigned BOOLEAN_TYPE
> conversion to/from precision 1 unsigned integral type is useless,
> but apparently the vectorizer heavily relies on the BOOLEAN_TYPE vs.
> unsigned integral:1 distinction (uses VECTOR_BOOLEAN_TYPE_P types
> only for real BOOLEAN_TYPE).  As the conversion is useless, we can end up
> as in the following testcase with e.g. binary operations which have boolean
> on one side and unsigned int:1 on another.
>
> The following patch replaces all explicit BOOLEAN_TYPE checks in the
> vectorizer with a new macro that handles unsigned integral:1 the same
> as BOOLEAN_TYPE.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2017-01-31  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR tree-optimization/79284
> 	* tree.h (INTEGRAL_BOOLEAN_TYPE_P): Define.
> 	* tree-vect-stmts.c (vect_get_vec_def_for_operand,
> 	vectorizable_mask_load_store, vectorizable_operation,
> 	vect_is_simple_cond, get_same_sized_vectype): Use it instead
> 	of comparing TREE_CODE of a type against BOOLEAN_TYPE.
> 	* tree-vect-patterns.c (check_bool_pattern, search_type_for_mask_1,
> 	vect_recog_bool_pattern, vect_recog_mask_conversion_pattern): Likewise.
> 	* tree-vect-slp.c (vect_get_constant_vectors): Likewise.
> 	* tree-vect-loop.c (vect_determine_vectorization_factor): Likewise.
Didn't we determine that the Ada front end generated boolean types with 
a precision greater than 1?   And if so, does that suggest that 
INTEGRAL_BOOLEAN_TYPE_P might need further adjustment (and that we 
likely have latent bugs in the vectorizer if it was presented with such 
types?)

jeff
Jakub Jelinek Jan. 31, 2017, 10:46 p.m. UTC | #2 
On Tue, Jan 31, 2017 at 03:41:27PM -0700, Jeff Law wrote:
> > useless_type_conversion_p says that precision 1 unsigned BOOLEAN_TYPE
> > conversion to/from precision 1 unsigned integral type is useless,
> > but apparently the vectorizer heavily relies on the BOOLEAN_TYPE vs.
> > unsigned integral:1 distinction (uses VECTOR_BOOLEAN_TYPE_P types
> > only for real BOOLEAN_TYPE).  As the conversion is useless, we can end up
> > as in the following testcase with e.g. binary operations which have boolean
> > on one side and unsigned int:1 on another.
> > 
> > The following patch replaces all explicit BOOLEAN_TYPE checks in the
> > vectorizer with a new macro that handles unsigned integral:1 the same
> > as BOOLEAN_TYPE.
> > 
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > 
> > 2017-01-31  Jakub Jelinek  <jakub@redhat.com>
> > 
> > 	PR tree-optimization/79284
> > 	* tree.h (INTEGRAL_BOOLEAN_TYPE_P): Define.
> > 	* tree-vect-stmts.c (vect_get_vec_def_for_operand,
> > 	vectorizable_mask_load_store, vectorizable_operation,
> > 	vect_is_simple_cond, get_same_sized_vectype): Use it instead
> > 	of comparing TREE_CODE of a type against BOOLEAN_TYPE.
> > 	* tree-vect-patterns.c (check_bool_pattern, search_type_for_mask_1,
> > 	vect_recog_bool_pattern, vect_recog_mask_conversion_pattern): Likewise.
> > 	* tree-vect-slp.c (vect_get_constant_vectors): Likewise.
> > 	* tree-vect-loop.c (vect_determine_vectorization_factor): Likewise.
> Didn't we determine that the Ada front end generated boolean types with a
> precision greater than 1?   And if so, does that suggest that
> INTEGRAL_BOOLEAN_TYPE_P might need further adjustment (and that we likely
> have latent bugs in the vectorizer if it was presented with such types?)

Some BOOLEAN_TYPE types (I think in Fortran too, though it might have
already changed) have bigger precision than one, but still have only 2 valid
values, 0 and 1 and I'm not aware of the vectorizer generating something
wrong for those.  For the non-BOOLEAN_TYPEs, only unsigned:1 is special:

  if (INTEGRAL_TYPE_P (inner_type)
      && INTEGRAL_TYPE_P (outer_type))
    {
      /* Preserve changes in signedness or precision.  */
      if (TYPE_UNSIGNED (inner_type) != TYPE_UNSIGNED (outer_type)
          || TYPE_PRECISION (inner_type) != TYPE_PRECISION (outer_type))
        return false;
      
      /* Preserve conversions to/from BOOLEAN_TYPE if types are not
         of precision one.  */
      if (((TREE_CODE (inner_type) == BOOLEAN_TYPE)
           != (TREE_CODE (outer_type) == BOOLEAN_TYPE))
          && TYPE_PRECISION (outer_type) != 1)
        return false;

      /* We don't need to preserve changes in the types minimum or
         maximum value in general as these do not generate code
         unless the types precisions are different.  */
      return true;
    }


	Jakub
Jeff Law Jan. 31, 2017, 10:52 p.m. UTC | #3 
On 01/31/2017 03:46 PM, Jakub Jelinek wrote:
> On Tue, Jan 31, 2017 at 03:41:27PM -0700, Jeff Law wrote:
>>> useless_type_conversion_p says that precision 1 unsigned BOOLEAN_TYPE
>>> conversion to/from precision 1 unsigned integral type is useless,
>>> but apparently the vectorizer heavily relies on the BOOLEAN_TYPE vs.
>>> unsigned integral:1 distinction (uses VECTOR_BOOLEAN_TYPE_P types
>>> only for real BOOLEAN_TYPE).  As the conversion is useless, we can end up
>>> as in the following testcase with e.g. binary operations which have boolean
>>> on one side and unsigned int:1 on another.
>>>
>>> The following patch replaces all explicit BOOLEAN_TYPE checks in the
>>> vectorizer with a new macro that handles unsigned integral:1 the same
>>> as BOOLEAN_TYPE.
>>>
>>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>>
>>> 2017-01-31  Jakub Jelinek  <jakub@redhat.com>
>>>
>>> 	PR tree-optimization/79284
>>> 	* tree.h (INTEGRAL_BOOLEAN_TYPE_P): Define.
>>> 	* tree-vect-stmts.c (vect_get_vec_def_for_operand,
>>> 	vectorizable_mask_load_store, vectorizable_operation,
>>> 	vect_is_simple_cond, get_same_sized_vectype): Use it instead
>>> 	of comparing TREE_CODE of a type against BOOLEAN_TYPE.
>>> 	* tree-vect-patterns.c (check_bool_pattern, search_type_for_mask_1,
>>> 	vect_recog_bool_pattern, vect_recog_mask_conversion_pattern): Likewise.
>>> 	* tree-vect-slp.c (vect_get_constant_vectors): Likewise.
>>> 	* tree-vect-loop.c (vect_determine_vectorization_factor): Likewise.
>> Didn't we determine that the Ada front end generated boolean types with a
>> precision greater than 1?   And if so, does that suggest that
>> INTEGRAL_BOOLEAN_TYPE_P might need further adjustment (and that we likely
>> have latent bugs in the vectorizer if it was presented with such types?)
>
> Some BOOLEAN_TYPE types (I think in Fortran too, though it might have
> already changed) have bigger precision than one, but still have only 2 valid
> values, 0 and 1 and I'm not aware of the vectorizer generating something
> wrong for those.  For the non-BOOLEAN_TYPEs, only unsigned:1 is special:
>
>   if (INTEGRAL_TYPE_P (inner_type)
>       && INTEGRAL_TYPE_P (outer_type))
>     {
>       /* Preserve changes in signedness or precision.  */
>       if (TYPE_UNSIGNED (inner_type) != TYPE_UNSIGNED (outer_type)
>           || TYPE_PRECISION (inner_type) != TYPE_PRECISION (outer_type))
>         return false;
>
>       /* Preserve conversions to/from BOOLEAN_TYPE if types are not
>          of precision one.  */
>       if (((TREE_CODE (inner_type) == BOOLEAN_TYPE)
>            != (TREE_CODE (outer_type) == BOOLEAN_TYPE))
>           && TYPE_PRECISION (outer_type) != 1)
>         return false;
>
>       /* We don't need to preserve changes in the types minimum or
>          maximum value in general as these do not generate code
>          unless the types precisions are different.  */
>       return true;
Which makes your patch safe -- but introduces a non-obvious dependency 
between useless_type_conversion_p and your definition of 
INTEGRAL_BOOLEAN_TYPE and how it's used in the vectorizer.

I think if you checked for a TYPE_PRECISION of 1 in 
INTEGRAL_BOOLEAN_TYPE, that would help -- but leaves 
INTEGRAL_BOOLEAN_TYPE poorly named.

Am I missing something here?

jeff
Jakub Jelinek Jan. 31, 2017, 11:22 p.m. UTC | #4 
On Tue, Jan 31, 2017 at 03:52:10PM -0700, Jeff Law wrote:
> Which makes your patch safe -- but introduces a non-obvious dependency
> between useless_type_conversion_p and your definition of
> INTEGRAL_BOOLEAN_TYPE and how it's used in the vectorizer.

The predicate is simply true for all BOOLEAN_TYPEs and all types that are
compatible with it in the middle-end.  BOOLEAN_TYPEs with different
precisions are not considered compatible types, therefore they won't appear
together without explicit casts in between.

> I think if you checked for a TYPE_PRECISION of 1 in INTEGRAL_BOOLEAN_TYPE,
> that would help -- but leaves INTEGRAL_BOOLEAN_TYPE poorly named.

I used BOOLEAN_TYPE_P initially, INTEGRAL_BOOLEAN_TYPE_P has been Richard's
suggestion on IRC.  I'm fine with other names as long as it is not too long.

	Jakub
Jeff Law Jan. 31, 2017, 11:31 p.m. UTC | #5 
On 01/31/2017 04:22 PM, Jakub Jelinek wrote:
> On Tue, Jan 31, 2017 at 03:52:10PM -0700, Jeff Law wrote:
>> Which makes your patch safe -- but introduces a non-obvious dependency
>> between useless_type_conversion_p and your definition of
>> INTEGRAL_BOOLEAN_TYPE and how it's used in the vectorizer.
>
> The predicate is simply true for all BOOLEAN_TYPEs and all types that are
> compatible with it in the middle-end.  BOOLEAN_TYPEs with different
> precisions are not considered compatible types, therefore they won't appear
> together without explicit casts in between.
I understand that.  My objection is that there's a highly non-obvious 
dependency between useless_type_conversion, INTEGRAL_TYPE_P (in 
particular how its used in the tree-vect-patterns).

If someone came along and changed useless_type_conversion, they'd have 
no way to know they need to go fix up INTEGRAL_TYPE_P and/or the 
vectorizer at the same time.

Thus my suggestion that you explicitly check the precision and rename 
the macro.  THough I don't offhand have a better suggestion.

Jeff
Jakub Jelinek Feb. 1, 2017, 8:01 a.m. UTC | #6 
On Tue, Jan 31, 2017 at 04:31:28PM -0700, Jeff Law wrote:
> On 01/31/2017 04:22 PM, Jakub Jelinek wrote:
> > On Tue, Jan 31, 2017 at 03:52:10PM -0700, Jeff Law wrote:
> > > Which makes your patch safe -- but introduces a non-obvious dependency
> > > between useless_type_conversion_p and your definition of
> > > INTEGRAL_BOOLEAN_TYPE and how it's used in the vectorizer.
> > 
> > The predicate is simply true for all BOOLEAN_TYPEs and all types that are
> > compatible with it in the middle-end.  BOOLEAN_TYPEs with different
> > precisions are not considered compatible types, therefore they won't appear
> > together without explicit casts in between.
> I understand that.  My objection is that there's a highly non-obvious
> dependency between useless_type_conversion, INTEGRAL_TYPE_P (in particular
> how its used in the tree-vect-patterns).
> 
> If someone came along and changed useless_type_conversion, they'd have no
> way to know they need to go fix up INTEGRAL_TYPE_P and/or the vectorizer at
> the same time.

I think we have many other places that depend on such behavior of u_t_c,
after all, the patch decreases the amount of such spots by replacing 3
places doing such checks manually with the macro that does the check.

> Thus my suggestion that you explicitly check the precision and rename the
> macro.  THough I don't offhand have a better suggestion.

Not sure I understand what you mean explicitly check the precision,
the macro checks the precision already, and intentionally only for
non-BOOLEAN_TYPE.  If you mean checking precision explicitly in the spots
where the macro is used, that would be worse for the hypothetical case when
u_t_c would change in this regard, you'd have far more places to change.
As for macro name, I came up e.g. with BOOLEAN_COMPATIBLE_P or
BOOLEAN_COMPATIBLE_TYPE_P, perhaps those could make it clearer on what it
is.

	Jakub
Richard Biener Feb. 1, 2017, 8:21 a.m. UTC | #7 
On Wed, 1 Feb 2017, Jakub Jelinek wrote:

> On Tue, Jan 31, 2017 at 04:31:28PM -0700, Jeff Law wrote:
> > On 01/31/2017 04:22 PM, Jakub Jelinek wrote:
> > > On Tue, Jan 31, 2017 at 03:52:10PM -0700, Jeff Law wrote:
> > > > Which makes your patch safe -- but introduces a non-obvious dependency
> > > > between useless_type_conversion_p and your definition of
> > > > INTEGRAL_BOOLEAN_TYPE and how it's used in the vectorizer.
> > > 
> > > The predicate is simply true for all BOOLEAN_TYPEs and all types that are
> > > compatible with it in the middle-end.  BOOLEAN_TYPEs with different
> > > precisions are not considered compatible types, therefore they won't appear
> > > together without explicit casts in between.
> > I understand that.  My objection is that there's a highly non-obvious
> > dependency between useless_type_conversion, INTEGRAL_TYPE_P (in particular
> > how its used in the tree-vect-patterns).
> > 
> > If someone came along and changed useless_type_conversion, they'd have no
> > way to know they need to go fix up INTEGRAL_TYPE_P and/or the vectorizer at
> > the same time.
> 
> I think we have many other places that depend on such behavior of u_t_c,
> after all, the patch decreases the amount of such spots by replacing 3
> places doing such checks manually with the macro that does the check.
> 
> > Thus my suggestion that you explicitly check the precision and rename the
> > macro.  THough I don't offhand have a better suggestion.
> 
> Not sure I understand what you mean explicitly check the precision,
> the macro checks the precision already, and intentionally only for
> non-BOOLEAN_TYPE.  If you mean checking precision explicitly in the spots
> where the macro is used, that would be worse for the hypothetical case when
> u_t_c would change in this regard, you'd have far more places to change.
> As for macro name, I came up e.g. with BOOLEAN_COMPATIBLE_P or
> BOOLEAN_COMPATIBLE_TYPE_P, perhaps those could make it clearer on what it
> is.

+/* Nonzero if TYPE represents a (scalar) boolean type or type
+   in the middle-end compatible with it.  */
+
+#define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
+  (TREE_CODE (TYPE) == BOOLEAN_TYPE            \
+   || ((TREE_CODE (TYPE) == INTEGER_TYPE       \
+       || TREE_CODE (TYPE) == ENUMERAL_TYPE)   \
+       && TYPE_PRECISION (TYPE) == 1           \
+       && TYPE_UNSIGNED (TYPE)))

(just to quote what you proposed).

As of useless_type_conversion_p, I don't remember why we have

      /* Preserve conversions to/from BOOLEAN_TYPE if types are not
         of precision one.  */
      if (((TREE_CODE (inner_type) == BOOLEAN_TYPE)
           != (TREE_CODE (outer_type) == BOOLEAN_TYPE))
          && TYPE_PRECISION (outer_type) != 1)
        return false;

it came with r173854 where you see other BOOLEAN_TYPE
-> integral-type with precision 1 check changes, so a new predicate
is very welcome IMHO.

all BOOLEAN_TYPEs but Adas have precision one and are unsigned
(their TYPE_SIZE may vary though).  Adas larger precision boolean
has only two valid values but needs to be able to encode some 'NaT'
state.

I think BOOLEAN_COMPATIBLE_TYPE_P would be misleading as it isn't
equal to types_compatible_p (boolean_type_node, t).

Maybe we want TWO_VALUED_UNSIGNED_INTEGRAL_TYPE_P ()? (ick)
I thought "BOOLEAN" covers TWO_VALUED_UNSIGNED well enough but
simply BOOLEAN_TYPE_P is easily confused with TREE_CODE () == 
BOOLEAN_TYPE.

I'm fine with changing the predicate to be more explicit, like

#define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
  (INTEGRAL_TYPE_P (TYPE) && TYPE_PRECISION (TYPE) == 1)

not sure if we really need the TYPE_UNSIGNED check?  The middle-end
has various places that just check for a 1-precision type when
asking for a boolean context.

So naming set aside, would you agree with the above definition?
(modulo a && TYPE_UNSIGNED (TYPE))?

Richard.
Jakub Jelinek Feb. 1, 2017, 8:30 a.m. UTC | #8 
On Wed, Feb 01, 2017 at 09:21:57AM +0100, Richard Biener wrote:
> it came with r173854 where you see other BOOLEAN_TYPE
> -> integral-type with precision 1 check changes, so a new predicate
> is very welcome IMHO.
> 
> all BOOLEAN_TYPEs but Adas have precision one and are unsigned
> (their TYPE_SIZE may vary though).  Adas larger precision boolean
> has only two valid values but needs to be able to encode some 'NaT'
> state.
> 
> I think BOOLEAN_COMPATIBLE_TYPE_P would be misleading as it isn't
> equal to types_compatible_p (boolean_type_node, t).
> 
> Maybe we want TWO_VALUED_UNSIGNED_INTEGRAL_TYPE_P ()? (ick)
> I thought "BOOLEAN" covers TWO_VALUED_UNSIGNED well enough but
> simply BOOLEAN_TYPE_P is easily confused with TREE_CODE () == 
> BOOLEAN_TYPE.
> 
> I'm fine with changing the predicate to be more explicit, like
> 
> #define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
>   (INTEGRAL_TYPE_P (TYPE) && TYPE_PRECISION (TYPE) == 1)
> 
> not sure if we really need the TYPE_UNSIGNED check?  The middle-end
> has various places that just check for a 1-precision type when
> asking for a boolean context.
> 
> So naming set aside, would you agree with the above definition?
> (modulo a && TYPE_UNSIGNED (TYPE))?

The TYPE_UNSIGNED check is there so that we don't consider signed 1 bit
bitfields as boolean, those are not compatible with any BOOLEAN_TYPE
(because they have different sign) and from the past experience, are
terribly hard to support properly ("true" is negative, is smaller than
"false", we had dozens of PRs about that kind of stuff already).

As for always checking TYPE_PRECISION == 1 and thus not considering
the Ada BOOLEAN_TYPE as something that should be vectorized as boolean
vector, I'm afraid (but haven't tried to prove that) it would make lots
of Ada testcases no longer vectorizable or vectorizable with much worse
code (especially on AVX512), and/or cause ICEs (if there are assumptions
e.g. that in COND_EXPR the first operand, if it is SSA_NAME, must be
vectorized as boolean vector type).

	Jakub
Eric Botcazou Feb. 1, 2017, 8:32 a.m. UTC | #9 
> all BOOLEAN_TYPEs but Adas have precision one and are unsigned
> (their TYPE_SIZE may vary though).

/* Builds a boolean type of precision PRECISION.
   Used for boolean vectors to choose proper vector element size.  */
tree
build_nonstandard_boolean_type (unsigned HOST_WIDE_INT precision)
{
  tree type;

  if (precision <= MAX_BOOL_CACHED_PREC)
    {
      type = nonstandard_boolean_type_cache[precision];
      if (type)
	return type;
    }

  type = make_node (BOOLEAN_TYPE);
  TYPE_PRECISION (type) = precision;
  fixup_signed_type (type);

  if (precision <= MAX_INT_CACHED_PREC)
    nonstandard_boolean_type_cache[precision] = type;

  return type;
}
Jakub Jelinek Feb. 1, 2017, 8:38 a.m. UTC | #10 
On Wed, Feb 01, 2017 at 09:32:46AM +0100, Eric Botcazou wrote:
> > all BOOLEAN_TYPEs but Adas have precision one and are unsigned
> > (their TYPE_SIZE may vary though).

Oops, thanks for the correction.  That said, the prec > 1 BOOLEAN_TYPE
aren't compatible with the prec 1 BOOLEAN_TYPE, so the intent of the
predicate is to support precision 1 BOOLEAN_TYPE and anything
types_compatible_p with it, or precision > 1 BOOLEAN_TYPEs (where
anything types_compatible_p with it must be BOOLEAN_TYPE with the
same precision and signedness).

> /* Builds a boolean type of precision PRECISION.
>    Used for boolean vectors to choose proper vector element size.  */
> tree
> build_nonstandard_boolean_type (unsigned HOST_WIDE_INT precision)
> {
>   tree type;
> 
>   if (precision <= MAX_BOOL_CACHED_PREC)
>     {
>       type = nonstandard_boolean_type_cache[precision];
>       if (type)
> 	return type;
>     }
> 
>   type = make_node (BOOLEAN_TYPE);
>   TYPE_PRECISION (type) = precision;
>   fixup_signed_type (type);
> 
>   if (precision <= MAX_INT_CACHED_PREC)
>     nonstandard_boolean_type_cache[precision] = type;
> 
>   return type;
> }

	Jakub
Jakub Jelinek Feb. 1, 2017, 9:50 a.m. UTC | #11 
On Wed, Feb 01, 2017 at 09:21:57AM +0100, Richard Biener wrote:
> > Not sure I understand what you mean explicitly check the precision,
> > the macro checks the precision already, and intentionally only for
> > non-BOOLEAN_TYPE.  If you mean checking precision explicitly in the spots
> > where the macro is used, that would be worse for the hypothetical case when
> > u_t_c would change in this regard, you'd have far more places to change.
> > As for macro name, I came up e.g. with BOOLEAN_COMPATIBLE_P or
> > BOOLEAN_COMPATIBLE_TYPE_P, perhaps those could make it clearer on what it
> > is.
> 
> +/* Nonzero if TYPE represents a (scalar) boolean type or type
> +   in the middle-end compatible with it.  */
> +
> +#define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
> +  (TREE_CODE (TYPE) == BOOLEAN_TYPE            \
> +   || ((TREE_CODE (TYPE) == INTEGER_TYPE       \
> +       || TREE_CODE (TYPE) == ENUMERAL_TYPE)   \
> +       && TYPE_PRECISION (TYPE) == 1           \
> +       && TYPE_UNSIGNED (TYPE)))
> 
> (just to quote what you proposed).

So would it help to use
  (TREE_CODE (TYPE) == BOOLEAN_TYPE
   || (INTEGRAL_TYPE_P (TYPE)
       && useless_type_conversion_p (boolean_type_node, TYPE)))
It would be much slower than the above, but would be less dependent
on useless_type_conversion_p details.

	Jakub
Richard Biener Feb. 1, 2017, 9:58 a.m. UTC | #12 
On Wed, 1 Feb 2017, Jakub Jelinek wrote:

> On Wed, Feb 01, 2017 at 09:21:57AM +0100, Richard Biener wrote:
> > > Not sure I understand what you mean explicitly check the precision,
> > > the macro checks the precision already, and intentionally only for
> > > non-BOOLEAN_TYPE.  If you mean checking precision explicitly in the spots
> > > where the macro is used, that would be worse for the hypothetical case when
> > > u_t_c would change in this regard, you'd have far more places to change.
> > > As for macro name, I came up e.g. with BOOLEAN_COMPATIBLE_P or
> > > BOOLEAN_COMPATIBLE_TYPE_P, perhaps those could make it clearer on what it
> > > is.
> > 
> > +/* Nonzero if TYPE represents a (scalar) boolean type or type
> > +   in the middle-end compatible with it.  */
> > +
> > +#define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
> > +  (TREE_CODE (TYPE) == BOOLEAN_TYPE            \
> > +   || ((TREE_CODE (TYPE) == INTEGER_TYPE       \
> > +       || TREE_CODE (TYPE) == ENUMERAL_TYPE)   \
> > +       && TYPE_PRECISION (TYPE) == 1           \
> > +       && TYPE_UNSIGNED (TYPE)))
> > 
> > (just to quote what you proposed).
> 
> So would it help to use
>   (TREE_CODE (TYPE) == BOOLEAN_TYPE
>    || (INTEGRAL_TYPE_P (TYPE)
>        && useless_type_conversion_p (boolean_type_node, TYPE)))
> It would be much slower than the above, but would be less dependent
> on useless_type_conversion_p details.

For the vectorizer it likely would break the larger logical type
handling?

The question is really what the vectorizer and other places are looking
for -- which isually is a 1-bit precision, eventually unsigned,
integral type.

I can't see where we'd use the variant with useless_type_conversion_p.

Richard.
Jakub Jelinek Feb. 1, 2017, 10:03 a.m. UTC | #13 
On Wed, Feb 01, 2017 at 10:58:29AM +0100, Richard Biener wrote:
> > > +/* Nonzero if TYPE represents a (scalar) boolean type or type
> > > +   in the middle-end compatible with it.  */
> > > +
> > > +#define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
> > > +  (TREE_CODE (TYPE) == BOOLEAN_TYPE            \
> > > +   || ((TREE_CODE (TYPE) == INTEGER_TYPE       \
> > > +       || TREE_CODE (TYPE) == ENUMERAL_TYPE)   \
> > > +       && TYPE_PRECISION (TYPE) == 1           \
> > > +       && TYPE_UNSIGNED (TYPE)))
> > > 
> > > (just to quote what you proposed).
> > 
> > So would it help to use
> >   (TREE_CODE (TYPE) == BOOLEAN_TYPE
> >    || (INTEGRAL_TYPE_P (TYPE)
> >        && useless_type_conversion_p (boolean_type_node, TYPE)))
> > It would be much slower than the above, but would be less dependent
> > on useless_type_conversion_p details.
> 
> For the vectorizer it likely would break the larger logical type
> handling?

Why?  It is the same thing as the earlier macro above.
Any kind of boolean, plus anything that could be initially boolean
and the middle-end might have replaced it with.

> The question is really what the vectorizer and other places are looking
> for -- which isually is a 1-bit precision, eventually unsigned,
> integral type.

It is looking for any type where the only valid values are 0 (false) and 1
(true), so that it can actually vectorize it as a bitmask, or vector of
integers with -1 and 0 values.

	Jakub
Richard Biener Feb. 1, 2017, 10:07 a.m. UTC | #14 
On Wed, 1 Feb 2017, Jakub Jelinek wrote:

> On Wed, Feb 01, 2017 at 10:58:29AM +0100, Richard Biener wrote:
> > > > +/* Nonzero if TYPE represents a (scalar) boolean type or type
> > > > +   in the middle-end compatible with it.  */
> > > > +
> > > > +#define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
> > > > +  (TREE_CODE (TYPE) == BOOLEAN_TYPE            \
> > > > +   || ((TREE_CODE (TYPE) == INTEGER_TYPE       \
> > > > +       || TREE_CODE (TYPE) == ENUMERAL_TYPE)   \
> > > > +       && TYPE_PRECISION (TYPE) == 1           \
> > > > +       && TYPE_UNSIGNED (TYPE)))
> > > > 
> > > > (just to quote what you proposed).
> > > 
> > > So would it help to use
> > >   (TREE_CODE (TYPE) == BOOLEAN_TYPE
> > >    || (INTEGRAL_TYPE_P (TYPE)
> > >        && useless_type_conversion_p (boolean_type_node, TYPE)))
> > > It would be much slower than the above, but would be less dependent
> > > on useless_type_conversion_p details.
> > 
> > For the vectorizer it likely would break the larger logical type
> > handling?
> 
> Why?  It is the same thing as the earlier macro above.
> Any kind of boolean, plus anything that could be initially boolean
> and the middle-end might have replaced it with.

boolean_type_node is QImode but logical(8) is DImode for example.
Both have precision == 1 but they are not types_compatible_p
(you probably missed the mode check in useless_type_conversion_p).

> > The question is really what the vectorizer and other places are looking
> > for -- which isually is a 1-bit precision, eventually unsigned,
> > integral type.
> 
> It is looking for any type where the only valid values are 0 (false) and 1
> (true), so that it can actually vectorize it as a bitmask, or vector of
> integers with -1 and 0 values.

That's INTEGRAL_TYPE_P && TYPE_PRECISION == 1 && TYPE_UNSIGNED.  The
Ada types do not fall under this category as far as I understand as
the exceptional values may exist in memory(?)

Richard.
Jakub Jelinek Feb. 1, 2017, 10:25 a.m. UTC | #15 
On Wed, Feb 01, 2017 at 11:07:18AM +0100, Richard Biener wrote:
> > On Wed, Feb 01, 2017 at 10:58:29AM +0100, Richard Biener wrote:
> > > > > +/* Nonzero if TYPE represents a (scalar) boolean type or type
> > > > > +   in the middle-end compatible with it.  */
> > > > > +
> > > > > +#define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
> > > > > +  (TREE_CODE (TYPE) == BOOLEAN_TYPE            \
> > > > > +   || ((TREE_CODE (TYPE) == INTEGER_TYPE       \
> > > > > +       || TREE_CODE (TYPE) == ENUMERAL_TYPE)   \
> > > > > +       && TYPE_PRECISION (TYPE) == 1           \
> > > > > +       && TYPE_UNSIGNED (TYPE)))
> > > > > 
> > > > > (just to quote what you proposed).
> > > > 
> > > > So would it help to use
> > > >   (TREE_CODE (TYPE) == BOOLEAN_TYPE
> > > >    || (INTEGRAL_TYPE_P (TYPE)
> > > >        && useless_type_conversion_p (boolean_type_node, TYPE)))
> > > > It would be much slower than the above, but would be less dependent
> > > > on useless_type_conversion_p details.
> > > 
> > > For the vectorizer it likely would break the larger logical type
> > > handling?
> > 
> > Why?  It is the same thing as the earlier macro above.
> > Any kind of boolean, plus anything that could be initially boolean
> > and the middle-end might have replaced it with.
> 
> boolean_type_node is QImode but logical(8) is DImode for example.
> Both have precision == 1 but they are not types_compatible_p
> (you probably missed the mode check in useless_type_conversion_p).

Then the earlier macro should have used also a TYPE_MODE check.
The latter macro is what I've been looking for in the patch, except that
I thought it is too expensive (plus might actually not DTRT if
boolean_type_node has > 1 precision, but some unsigned precision 1
BOOLEAN_TYPE exists too; as Fortran has changed, now it is only Ada that
doesn't have precision 1 boolean_type_node, but then it likely doesn't
have any precision 1 BOOLEAN_TYPEs).

> > > The question is really what the vectorizer and other places are looking
> > > for -- which isually is a 1-bit precision, eventually unsigned,
> > > integral type.
> > 
> > It is looking for any type where the only valid values are 0 (false) and 1
> > (true), so that it can actually vectorize it as a bitmask, or vector of
> > integers with -1 and 0 values.
> 
> That's INTEGRAL_TYPE_P && TYPE_PRECISION == 1 && TYPE_UNSIGNED.  The
> Ada types do not fall under this category as far as I understand as
> the exceptional values may exist in memory(?)

But the exceptional values are undefined behavior I believe.
Anyway, we've been vectorizing not just the 1-bit precision BOOLEAN_TYPEs
but other BOOLEAN_TYPEs (Ada and especially Fortran) for a couple of years
this way already and I'm not aware of issues with that.

So suddenly stopping doing that because we want to fix a bug related
to the fact that for 1-bit precision unsigned QImode BOOLEAN_TYPE the
middle-end considers other types to be compatible with those is strange
and very risky, especially at this point in GCC 7 development.

	Jakub
Richard Biener Feb. 1, 2017, 10:45 a.m. UTC | #16 
On Wed, 1 Feb 2017, Jakub Jelinek wrote:

> On Wed, Feb 01, 2017 at 11:07:18AM +0100, Richard Biener wrote:
> > > On Wed, Feb 01, 2017 at 10:58:29AM +0100, Richard Biener wrote:
> > > > > > +/* Nonzero if TYPE represents a (scalar) boolean type or type
> > > > > > +   in the middle-end compatible with it.  */
> > > > > > +
> > > > > > +#define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
> > > > > > +  (TREE_CODE (TYPE) == BOOLEAN_TYPE            \
> > > > > > +   || ((TREE_CODE (TYPE) == INTEGER_TYPE       \
> > > > > > +       || TREE_CODE (TYPE) == ENUMERAL_TYPE)   \
> > > > > > +       && TYPE_PRECISION (TYPE) == 1           \
> > > > > > +       && TYPE_UNSIGNED (TYPE)))
> > > > > > 
> > > > > > (just to quote what you proposed).
> > > > > 
> > > > > So would it help to use
> > > > >   (TREE_CODE (TYPE) == BOOLEAN_TYPE
> > > > >    || (INTEGRAL_TYPE_P (TYPE)
> > > > >        && useless_type_conversion_p (boolean_type_node, TYPE)))
> > > > > It would be much slower than the above, but would be less dependent
> > > > > on useless_type_conversion_p details.
> > > > 
> > > > For the vectorizer it likely would break the larger logical type
> > > > handling?
> > > 
> > > Why?  It is the same thing as the earlier macro above.
> > > Any kind of boolean, plus anything that could be initially boolean
> > > and the middle-end might have replaced it with.
> > 
> > boolean_type_node is QImode but logical(8) is DImode for example.
> > Both have precision == 1 but they are not types_compatible_p
> > (you probably missed the mode check in useless_type_conversion_p).
> 
> Then the earlier macro should have used also a TYPE_MODE check.
> The latter macro is what I've been looking for in the patch, except that
> I thought it is too expensive (plus might actually not DTRT if
> boolean_type_node has > 1 precision, but some unsigned precision 1
> BOOLEAN_TYPE exists too; as Fortran has changed, now it is only Ada that
> doesn't have precision 1 boolean_type_node, but then it likely doesn't
> have any precision 1 BOOLEAN_TYPEs).
> 
> > > > The question is really what the vectorizer and other places are looking
> > > > for -- which isually is a 1-bit precision, eventually unsigned,
> > > > integral type.
> > > 
> > > It is looking for any type where the only valid values are 0 (false) and 1
> > > (true), so that it can actually vectorize it as a bitmask, or vector of
> > > integers with -1 and 0 values.
> > 
> > That's INTEGRAL_TYPE_P && TYPE_PRECISION == 1 && TYPE_UNSIGNED.  The
> > Ada types do not fall under this category as far as I understand as
> > the exceptional values may exist in memory(?)
> 
> But the exceptional values are undefined behavior I believe.
> Anyway, we've been vectorizing not just the 1-bit precision BOOLEAN_TYPEs
> but other BOOLEAN_TYPEs (Ada and especially Fortran) for a couple of years
> this way already and I'm not aware of issues with that.
> 
> So suddenly stopping doing that because we want to fix a bug related
> to the fact that for 1-bit precision unsigned QImode BOOLEAN_TYPE the
> middle-end considers other types to be compatible with those is strange
> and very risky, especially at this point in GCC 7 development.

I agree.  But this means we should look for a vectorizer-local fix
without a new global predicate then (there seem to be subtly different
needs and coming up with good names for all of them sounds difficult...).

Richard.
Jeff Law Feb. 2, 2017, 5:07 p.m. UTC | #17 
On 02/01/2017 01:21 AM, Richard Biener wrote:
>
> +/* Nonzero if TYPE represents a (scalar) boolean type or type
> +   in the middle-end compatible with it.  */
> +
> +#define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
> +  (TREE_CODE (TYPE) == BOOLEAN_TYPE            \
> +   || ((TREE_CODE (TYPE) == INTEGER_TYPE       \
> +       || TREE_CODE (TYPE) == ENUMERAL_TYPE)   \
> +       && TYPE_PRECISION (TYPE) == 1           \
> +       && TYPE_UNSIGNED (TYPE)))
>
> (just to quote what you proposed).
And my suggestion is to move the PRECISION/UNSIGNED checks so that 
they're tested for BOOLEAN_TYPEs as well.


>
> As of useless_type_conversion_p, I don't remember why we have
>
>       /* Preserve conversions to/from BOOLEAN_TYPE if types are not
>          of precision one.  */
>       if (((TREE_CODE (inner_type) == BOOLEAN_TYPE)
>            != (TREE_CODE (outer_type) == BOOLEAN_TYPE))
>           && TYPE_PRECISION (outer_type) != 1)
>         return false;
>
> it came with r173854 where you see other BOOLEAN_TYPE
> -> integral-type with precision 1 check changes, so a new predicate
> is very welcome IMHO.
Funny, it's your change ;-)  Looks like Kai added a gimple checker that 
verified that the operand of a TRUTH_NOT_EXPR must be compatible with 
boolean_type_node.

That check tripped (bz48989) on some Fortran code.  Your fix seems to 
imply that the Fortran front-end created non-1-precision BOOLEAN_TYPE 
object.


>
> all BOOLEAN_TYPEs but Adas have precision one and are unsigned
> (their TYPE_SIZE may vary though).  Adas larger precision boolean
> has only two valid values but needs to be able to encode some 'NaT'
> state.
>
> I think BOOLEAN_COMPATIBLE_TYPE_P would be misleading as it isn't
> equal to types_compatible_p (boolean_type_node, t).
>
> Maybe we want TWO_VALUED_UNSIGNED_INTEGRAL_TYPE_P ()? (ick)
> I thought "BOOLEAN" covers TWO_VALUED_UNSIGNED well enough but
> simply BOOLEAN_TYPE_P is easily confused with TREE_CODE () ==
> BOOLEAN_TYPE.
>
> I'm fine with changing the predicate to be more explicit, like
>
> #define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
>   (INTEGRAL_TYPE_P (TYPE) && TYPE_PRECISION (TYPE) == 1)
>
> not sure if we really need the TYPE_UNSIGNED check?  The middle-end
> has various places that just check for a 1-precision type when
> asking for a boolean context.
>
> So naming set aside, would you agree with the above definition?
> (modulo a && TYPE_UNSIGNED (TYPE))?
I could agree to that.  Alternately, we could restore the TYPE_PRECISION 
checks that Jakub removed in the vectorizer.

Jeff
Jeff Law Feb. 2, 2017, 5:12 p.m. UTC | #18 
On 02/01/2017 03:45 AM, Richard Biener wrote:
>
> I agree.  But this means we should look for a vectorizer-local fix
> without a new global predicate then (there seem to be subtly different
> needs and coming up with good names for all of them sounds difficult...).
Well, we could go with Jakub's INTEGRAL_BOOLEAN_TYPE as posted, but in 
contexts where we use it and really depend on single bit objects, we add 
the precision == 1 check back.  Jakub's patch removes the type precision 
check in tree-vect-patterns for example.  There's likely all kinds of 
places where we need to add that check as well.

/me is frustrated that we have booleans with nonstandard precision, even 
though I understand why it was done.  It creates numerous headaches.


Jeff
Jakub Jelinek Feb. 2, 2017, 10:32 p.m. UTC | #19 
On Thu, Feb 02, 2017 at 10:12:32AM -0700, Jeff Law wrote:
> On 02/01/2017 03:45 AM, Richard Biener wrote:
> > 
> > I agree.  But this means we should look for a vectorizer-local fix
> > without a new global predicate then (there seem to be subtly different
> > needs and coming up with good names for all of them sounds difficult...).
> Well, we could go with Jakub's INTEGRAL_BOOLEAN_TYPE as posted, but in
> contexts where we use it and really depend on single bit objects, we add the
> precision == 1 check back.  Jakub's patch removes the type precision check
> in tree-vect-patterns for example.  There's likely all kinds of places where
> we need to add that check as well.

The 3 cases in tree-vect-patterns.c where I've removed the check were
exactly what the proposed macro does, i.e.
      if ((TYPE_PRECISION (TREE_TYPE (rhs1)) != 1
           || !TYPE_UNSIGNED (TREE_TYPE (rhs1)))
          && TREE_CODE (TREE_TYPE (rhs1)) != BOOLEAN_TYPE)
	return false;
i.e. bail out unless the rhs1 type is a BOOLEAN_TYPE (any precision,
assuming it has only valid values of 0 and 1) or unless it is unsigned
precision type 1 integer (i.e. something that (if it has also QImode)
forwprop etc. could have changed a BOOLEAN_TYPE with precision 1 into.

Note Fortran has been using I think precision one boolean_type_node even in GCC 6.x,
it had:
  boolean_type_node = gfc_get_logical_type (gfc_default_logical_kind);
  boolean_true_node = build_int_cst (boolean_type_node, 1);
  boolean_false_node = build_int_cst (boolean_type_node, 0);
Fortran logical(4) etc. are built using:
  new_type = make_unsigned_type (bit_size);
  TREE_SET_CODE (new_type, BOOLEAN_TYPE);
  TYPE_MAX_VALUE (new_type) = build_int_cst (new_type, 1);
  TYPE_PRECISION (new_type) = 1;
thus I believe they have e.g. SImode or DImode rather than QImode, but still
TYPE_PRECISION of 1.  The non-Ada boolean is also:
  boolean_type_node = make_unsigned_type (BOOL_TYPE_SIZE);
  TREE_SET_CODE (boolean_type_node, BOOLEAN_TYPE);
  TYPE_PRECISION (boolean_type_node) = 1;
where BOOL_TYPE_SIZE is CHAR_TYPE_SIZE everywhere but on powerpc*-darwin*
with some command line option.
Ada is
  boolean_type_node = make_unsigned_type (8);
  TREE_SET_CODE (boolean_type_node, BOOLEAN_TYPE);
and thus it is indeed precision 8 QImode.

So, requiring precision 1 for all BOOLEAN_TYPEs in the vectorizer would
only affect Ada.  Requiring QImode or whatever other fixed TYPE_MODE
in the macro for precision 1 unsigned non-BOOLEAN_TYPE integers would
actually also affect Fortran a lot, because e.g. SImode precision 1
INTEGER_TYPE is considered compatible with SImode precision 1 BOOLEAN_TYPE.

> /me is frustrated that we have booleans with nonstandard precision, even
> though I understand why it was done.  It creates numerous headaches.

Ditto.

	Jakub
Richard Biener Feb. 6, 2017, 8:14 a.m. UTC | #20 
On Thu, 2 Feb 2017, Jeff Law wrote:

> On 02/01/2017 01:21 AM, Richard Biener wrote:
> > 
> > +/* Nonzero if TYPE represents a (scalar) boolean type or type
> > +   in the middle-end compatible with it.  */
> > +
> > +#define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
> > +  (TREE_CODE (TYPE) == BOOLEAN_TYPE            \
> > +   || ((TREE_CODE (TYPE) == INTEGER_TYPE       \
> > +       || TREE_CODE (TYPE) == ENUMERAL_TYPE)   \
> > +       && TYPE_PRECISION (TYPE) == 1           \
> > +       && TYPE_UNSIGNED (TYPE)))
> > 
> > (just to quote what you proposed).
> And my suggestion is to move the PRECISION/UNSIGNED checks so that they're
> tested for BOOLEAN_TYPEs as well.
> 
> 
> > 
> > As of useless_type_conversion_p, I don't remember why we have
> > 
> >       /* Preserve conversions to/from BOOLEAN_TYPE if types are not
> >          of precision one.  */
> >       if (((TREE_CODE (inner_type) == BOOLEAN_TYPE)
> >            != (TREE_CODE (outer_type) == BOOLEAN_TYPE))
> >           && TYPE_PRECISION (outer_type) != 1)
> >         return false;
> > 
> > it came with r173854 where you see other BOOLEAN_TYPE
> > -> integral-type with precision 1 check changes, so a new predicate
> > is very welcome IMHO.
> Funny, it's your change ;-)  Looks like Kai added a gimple checker that
> verified that the operand of a TRUTH_NOT_EXPR must be compatible with
> boolean_type_node.
> 
> That check tripped (bz48989) on some Fortran code.  Your fix seems to imply
> that the Fortran front-end created non-1-precision BOOLEAN_TYPE object.

It used to override boolean_type_node which AFAIK it no longer does.

> 
> > 
> > all BOOLEAN_TYPEs but Adas have precision one and are unsigned
> > (their TYPE_SIZE may vary though).  Adas larger precision boolean
> > has only two valid values but needs to be able to encode some 'NaT'
> > state.
> > 
> > I think BOOLEAN_COMPATIBLE_TYPE_P would be misleading as it isn't
> > equal to types_compatible_p (boolean_type_node, t).
> > 
> > Maybe we want TWO_VALUED_UNSIGNED_INTEGRAL_TYPE_P ()? (ick)
> > I thought "BOOLEAN" covers TWO_VALUED_UNSIGNED well enough but
> > simply BOOLEAN_TYPE_P is easily confused with TREE_CODE () ==
> > BOOLEAN_TYPE.
> > 
> > I'm fine with changing the predicate to be more explicit, like
> > 
> > #define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
> >   (INTEGRAL_TYPE_P (TYPE) && TYPE_PRECISION (TYPE) == 1)
> > 
> > not sure if we really need the TYPE_UNSIGNED check?  The middle-end
> > has various places that just check for a 1-precision type when
> > asking for a boolean context.
> > 
> > So naming set aside, would you agree with the above definition?
> > (modulo a && TYPE_UNSIGNED (TYPE))?
> I could agree to that.  Alternately, we could restore the TYPE_PRECISION
> checks that Jakub removed in the vectorizer.

Yeah, I guess at this point I prefer a vectorizer-local fix.

Richard.
Jakub Jelinek Feb. 6, 2017, 8:30 a.m. UTC | #21 
On Mon, Feb 06, 2017 at 09:14:24AM +0100, Richard Biener wrote:
> > > +/* Nonzero if TYPE represents a (scalar) boolean type or type
> > > +   in the middle-end compatible with it.  */
> > > +
> > > +#define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
> > > +  (TREE_CODE (TYPE) == BOOLEAN_TYPE            \
> > > +   || ((TREE_CODE (TYPE) == INTEGER_TYPE       \
> > > +       || TREE_CODE (TYPE) == ENUMERAL_TYPE)   \
> > > +       && TYPE_PRECISION (TYPE) == 1           \
> > > +       && TYPE_UNSIGNED (TYPE)))
> > > 
> > > (just to quote what you proposed).

> > I could agree to that.  Alternately, we could restore the TYPE_PRECISION
> > checks that Jakub removed in the vectorizer.

I haven't removed any, there were just 3 changes of the kind:
-  if ((TYPE_PRECISION (TREE_TYPE (var)) != 1
-       || !TYPE_UNSIGNED (TREE_TYPE (var)))
-      && TREE_CODE (TREE_TYPE (var)) != BOOLEAN_TYPE)
+  if (!INTEGRAL_BOOLEAN_TYPE_P (TREE_TYPE (var)))
which are almost equivalent, except that the old code could let
non-INTEGRAL_TYPE_P with TYPE_PRECISION (whatever it means
for the various kinds of types (e.g. log2 of number of vector elements
etc.).

> 
> Yeah, I guess at this point I prefer a vectorizer-local fix.

So shall I move the INTEGRAL_BOOLEAN_TYPE_P macro (or change it to
another name, e.g. including VECT in it?) to tree-vectorizer.h?

	Jakub
Richard Biener Feb. 6, 2017, 2:23 p.m. UTC | #22 
On Thu, 2 Feb 2017, Jakub Jelinek wrote:

> On Thu, Feb 02, 2017 at 10:12:32AM -0700, Jeff Law wrote:
> > On 02/01/2017 03:45 AM, Richard Biener wrote:
> > > 
> > > I agree.  But this means we should look for a vectorizer-local fix
> > > without a new global predicate then (there seem to be subtly different
> > > needs and coming up with good names for all of them sounds difficult...).
> > Well, we could go with Jakub's INTEGRAL_BOOLEAN_TYPE as posted, but in
> > contexts where we use it and really depend on single bit objects, we add the
> > precision == 1 check back.  Jakub's patch removes the type precision check
> > in tree-vect-patterns for example.  There's likely all kinds of places where
> > we need to add that check as well.
> 
> The 3 cases in tree-vect-patterns.c where I've removed the check were
> exactly what the proposed macro does, i.e.
>       if ((TYPE_PRECISION (TREE_TYPE (rhs1)) != 1
>            || !TYPE_UNSIGNED (TREE_TYPE (rhs1)))
>           && TREE_CODE (TREE_TYPE (rhs1)) != BOOLEAN_TYPE)
> 	return false;
> i.e. bail out unless the rhs1 type is a BOOLEAN_TYPE (any precision,
> assuming it has only valid values of 0 and 1) or unless it is unsigned
> precision type 1 integer (i.e. something that (if it has also QImode)
> forwprop etc. could have changed a BOOLEAN_TYPE with precision 1 into.
> 
> Note Fortran has been using I think precision one boolean_type_node even in GCC 6.x,
> it had:
>   boolean_type_node = gfc_get_logical_type (gfc_default_logical_kind);
>   boolean_true_node = build_int_cst (boolean_type_node, 1);
>   boolean_false_node = build_int_cst (boolean_type_node, 0);
> Fortran logical(4) etc. are built using:
>   new_type = make_unsigned_type (bit_size);
>   TREE_SET_CODE (new_type, BOOLEAN_TYPE);
>   TYPE_MAX_VALUE (new_type) = build_int_cst (new_type, 1);
>   TYPE_PRECISION (new_type) = 1;
> thus I believe they have e.g. SImode or DImode rather than QImode, but still
> TYPE_PRECISION of 1.  The non-Ada boolean is also:
>   boolean_type_node = make_unsigned_type (BOOL_TYPE_SIZE);
>   TREE_SET_CODE (boolean_type_node, BOOLEAN_TYPE);
>   TYPE_PRECISION (boolean_type_node) = 1;
> where BOOL_TYPE_SIZE is CHAR_TYPE_SIZE everywhere but on powerpc*-darwin*
> with some command line option.
> Ada is
>   boolean_type_node = make_unsigned_type (8);
>   TREE_SET_CODE (boolean_type_node, BOOLEAN_TYPE);
> and thus it is indeed precision 8 QImode.
> 
> So, requiring precision 1 for all BOOLEAN_TYPEs in the vectorizer would
> only affect Ada.  Requiring QImode or whatever other fixed TYPE_MODE
> in the macro for precision 1 unsigned non-BOOLEAN_TYPE integers would
> actually also affect Fortran a lot, because e.g. SImode precision 1
> INTEGER_TYPE is considered compatible with SImode precision 1 BOOLEAN_TYPE.
> 
> > /me is frustrated that we have booleans with nonstandard precision, even
> > though I understand why it was done.  It creates numerous headaches.
> 
> Ditto.

I guess I'm mostly frustrated that we have BOOLEAN_TYPE/ENUMERAL_TYPE at
all given precision-1 unsigned INTEGER_TYPEs are fine to represent
0/1 valued entities (aka bools).  But then we also have BImode...

Richard.
Richard Biener Feb. 6, 2017, 2:26 p.m. UTC | #23 
On Mon, 6 Feb 2017, Jakub Jelinek wrote:

> On Mon, Feb 06, 2017 at 09:14:24AM +0100, Richard Biener wrote:
> > > > +/* Nonzero if TYPE represents a (scalar) boolean type or type
> > > > +   in the middle-end compatible with it.  */
> > > > +
> > > > +#define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
> > > > +  (TREE_CODE (TYPE) == BOOLEAN_TYPE            \
> > > > +   || ((TREE_CODE (TYPE) == INTEGER_TYPE       \
> > > > +       || TREE_CODE (TYPE) == ENUMERAL_TYPE)   \
> > > > +       && TYPE_PRECISION (TYPE) == 1           \
> > > > +       && TYPE_UNSIGNED (TYPE)))
> > > > 
> > > > (just to quote what you proposed).
> 
> > > I could agree to that.  Alternately, we could restore the TYPE_PRECISION
> > > checks that Jakub removed in the vectorizer.
> 
> I haven't removed any, there were just 3 changes of the kind:
> -  if ((TYPE_PRECISION (TREE_TYPE (var)) != 1
> -       || !TYPE_UNSIGNED (TREE_TYPE (var)))
> -      && TREE_CODE (TREE_TYPE (var)) != BOOLEAN_TYPE)
> +  if (!INTEGRAL_BOOLEAN_TYPE_P (TREE_TYPE (var)))
> which are almost equivalent, except that the old code could let
> non-INTEGRAL_TYPE_P with TYPE_PRECISION (whatever it means
> for the various kinds of types (e.g. log2 of number of vector elements
> etc.).
> 
> > 
> > Yeah, I guess at this point I prefer a vectorizer-local fix.
> 
> So shall I move the INTEGRAL_BOOLEAN_TYPE_P macro (or change it to
> another name, e.g. including VECT in it?) to tree-vectorizer.h?

Works for me.  Like VECT_SCALAR_BOOLEAN_TYPE_P () to not confuse it
with VECTOR_BOOLEAN_TYPE_P?

Richard.
Jeff Law Feb. 6, 2017, 3:35 p.m. UTC | #24 
On 02/06/2017 07:23 AM, Richard Biener wrote:
> On Thu, 2 Feb 2017, Jakub Jelinek wrote:
>
>> On Thu, Feb 02, 2017 at 10:12:32AM -0700, Jeff Law wrote:
>>> On 02/01/2017 03:45 AM, Richard Biener wrote:
>>>>
>>>> I agree.  But this means we should look for a vectorizer-local fix
>>>> without a new global predicate then (there seem to be subtly different
>>>> needs and coming up with good names for all of them sounds difficult...).
>>> Well, we could go with Jakub's INTEGRAL_BOOLEAN_TYPE as posted, but in
>>> contexts where we use it and really depend on single bit objects, we add the
>>> precision == 1 check back.  Jakub's patch removes the type precision check
>>> in tree-vect-patterns for example.  There's likely all kinds of places where
>>> we need to add that check as well.
>>
>> The 3 cases in tree-vect-patterns.c where I've removed the check were
>> exactly what the proposed macro does, i.e.
>>       if ((TYPE_PRECISION (TREE_TYPE (rhs1)) != 1
>>            || !TYPE_UNSIGNED (TREE_TYPE (rhs1)))
>>           && TREE_CODE (TREE_TYPE (rhs1)) != BOOLEAN_TYPE)
>> 	return false;
>> i.e. bail out unless the rhs1 type is a BOOLEAN_TYPE (any precision,
>> assuming it has only valid values of 0 and 1) or unless it is unsigned
>> precision type 1 integer (i.e. something that (if it has also QImode)
>> forwprop etc. could have changed a BOOLEAN_TYPE with precision 1 into.
>>
>> Note Fortran has been using I think precision one boolean_type_node even in GCC 6.x,
>> it had:
>>   boolean_type_node = gfc_get_logical_type (gfc_default_logical_kind);
>>   boolean_true_node = build_int_cst (boolean_type_node, 1);
>>   boolean_false_node = build_int_cst (boolean_type_node, 0);
>> Fortran logical(4) etc. are built using:
>>   new_type = make_unsigned_type (bit_size);
>>   TREE_SET_CODE (new_type, BOOLEAN_TYPE);
>>   TYPE_MAX_VALUE (new_type) = build_int_cst (new_type, 1);
>>   TYPE_PRECISION (new_type) = 1;
>> thus I believe they have e.g. SImode or DImode rather than QImode, but still
>> TYPE_PRECISION of 1.  The non-Ada boolean is also:
>>   boolean_type_node = make_unsigned_type (BOOL_TYPE_SIZE);
>>   TREE_SET_CODE (boolean_type_node, BOOLEAN_TYPE);
>>   TYPE_PRECISION (boolean_type_node) = 1;
>> where BOOL_TYPE_SIZE is CHAR_TYPE_SIZE everywhere but on powerpc*-darwin*
>> with some command line option.
>> Ada is
>>   boolean_type_node = make_unsigned_type (8);
>>   TREE_SET_CODE (boolean_type_node, BOOLEAN_TYPE);
>> and thus it is indeed precision 8 QImode.
>>
>> So, requiring precision 1 for all BOOLEAN_TYPEs in the vectorizer would
>> only affect Ada.  Requiring QImode or whatever other fixed TYPE_MODE
>> in the macro for precision 1 unsigned non-BOOLEAN_TYPE integers would
>> actually also affect Fortran a lot, because e.g. SImode precision 1
>> INTEGER_TYPE is considered compatible with SImode precision 1 BOOLEAN_TYPE.
>>
>>> /me is frustrated that we have booleans with nonstandard precision, even
>>> though I understand why it was done.  It creates numerous headaches.
>>
>> Ditto.
>
> I guess I'm mostly frustrated that we have BOOLEAN_TYPE/ENUMERAL_TYPE at
> all given precision-1 unsigned INTEGER_TYPEs are fine to represent
> 0/1 valued entities (aka bools).  But then we also have BImode...
IIRC BImode was the most natural way to support predication on ia64 and 
likely could be used in that manner on other targets if we wanted to get 
serious about predication.

jeff
Jeff Law Feb. 6, 2017, 3:39 p.m. UTC | #25 
On 02/02/2017 03:32 PM, Jakub Jelinek wrote:
> On Thu, Feb 02, 2017 at 10:12:32AM -0700, Jeff Law wrote:
>> On 02/01/2017 03:45 AM, Richard Biener wrote:
>>>
>>> I agree.  But this means we should look for a vectorizer-local fix
>>> without a new global predicate then (there seem to be subtly different
>>> needs and coming up with good names for all of them sounds difficult...).
>> Well, we could go with Jakub's INTEGRAL_BOOLEAN_TYPE as posted, but in
>> contexts where we use it and really depend on single bit objects, we add the
>> precision == 1 check back.  Jakub's patch removes the type precision check
>> in tree-vect-patterns for example.  There's likely all kinds of places where
>> we need to add that check as well.
>
> The 3 cases in tree-vect-patterns.c where I've removed the check were
> exactly what the proposed macro does, i.e.
>       if ((TYPE_PRECISION (TREE_TYPE (rhs1)) != 1
>            || !TYPE_UNSIGNED (TREE_TYPE (rhs1)))
>           && TREE_CODE (TREE_TYPE (rhs1)) != BOOLEAN_TYPE)
> 	return false;
> i.e. bail out unless the rhs1 type is a BOOLEAN_TYPE (any precision,
> assuming it has only valid values of 0 and 1) or unless it is unsigned
> precision type 1 integer (i.e. something that (if it has also QImode)
> forwprop etc. could have changed a BOOLEAN_TYPE with precision 1 into.
Doh!  I should have read those hunks more closely.  Sorry for creating 
the lengthier than necessary discussion.

Jeff
diff mbox

Patch

--- gcc/tree.h.jj	2017-01-23 23:57:41.000000000 +0100
+++ gcc/tree.h	2017-01-31 16:16:28.969224648 +0100
@@ -532,6 +532,16 @@  extern void omp_clause_range_check_faile
         || VECTOR_TYPE_P (TYPE))		\
        && INTEGRAL_TYPE_P (TREE_TYPE (TYPE))))
 
+/* Nonzero if TYPE represents a (scalar) boolean type or type
+   in the middle-end compatible with it.  */
+
+#define INTEGRAL_BOOLEAN_TYPE_P(TYPE) \
+  (TREE_CODE (TYPE) == BOOLEAN_TYPE		\
+   || ((TREE_CODE (TYPE) == INTEGER_TYPE	\
+	|| TREE_CODE (TYPE) == ENUMERAL_TYPE)	\
+       && TYPE_PRECISION (TYPE) == 1		\
+       && TYPE_UNSIGNED (TYPE)))
+
 /* Nonzero if TYPE represents a non-saturating fixed-point type.  */
 
 #define NON_SAT_FIXED_POINT_TYPE_P(TYPE) \
--- gcc/tree-vect-stmts.c.jj	2017-01-09 12:44:14.000000000 +0100
+++ gcc/tree-vect-stmts.c	2017-01-31 16:18:51.855369244 +0100
@@ -1420,7 +1420,7 @@  vect_get_vec_def_for_operand (tree op, g
 
       if (vectype)
 	vector_type = vectype;
-      else if (TREE_CODE (TREE_TYPE (op)) == BOOLEAN_TYPE
+      else if (INTEGRAL_BOOLEAN_TYPE_P (TREE_TYPE (op))
 	       && VECTOR_BOOLEAN_TYPE_P (stmt_vectype))
 	vector_type = build_same_sized_truth_vector_type (stmt_vectype);
       else
@@ -2029,7 +2029,7 @@  vectorizable_mask_load_store (gimple *st
 
   mask = gimple_call_arg (stmt, 2);
 
-  if (TREE_CODE (TREE_TYPE (mask)) != BOOLEAN_TYPE)
+  if (!INTEGRAL_BOOLEAN_TYPE_P (TREE_TYPE (mask)))
     return false;
 
   /* FORNOW. This restriction should be relaxed.  */
@@ -5275,9 +5275,9 @@  vectorizable_operation (gimple *stmt, gi
 	 of booleans or vector of integers).  We use output
 	 vectype because operations on boolean don't change
 	 type.  */
-      if (TREE_CODE (TREE_TYPE (op0)) == BOOLEAN_TYPE)
+      if (INTEGRAL_BOOLEAN_TYPE_P (TREE_TYPE (op0)))
 	{
-	  if (TREE_CODE (TREE_TYPE (scalar_dest)) != BOOLEAN_TYPE)
+	  if (!INTEGRAL_BOOLEAN_TYPE_P (TREE_TYPE (scalar_dest)))
 	    {
 	      if (dump_enabled_p ())
 		dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
@@ -7666,7 +7666,7 @@  vect_is_simple_cond (tree cond, vec_info
 
   /* Mask case.  */
   if (TREE_CODE (cond) == SSA_NAME
-      && TREE_CODE (TREE_TYPE (cond)) == BOOLEAN_TYPE)
+      && INTEGRAL_BOOLEAN_TYPE_P (TREE_TYPE (cond)))
     {
       gimple *lhs_def_stmt = SSA_NAME_DEF_STMT (cond);
       if (!vect_is_simple_use (cond, vinfo, &lhs_def_stmt,
@@ -9059,7 +9059,7 @@  get_mask_type_for_scalar_type (tree scal
 tree
 get_same_sized_vectype (tree scalar_type, tree vector_type)
 {
-  if (TREE_CODE (scalar_type) == BOOLEAN_TYPE)
+  if (INTEGRAL_BOOLEAN_TYPE_P (scalar_type))
     return build_same_sized_truth_vector_type (vector_type);
 
   return get_vectype_for_scalar_type_and_size
--- gcc/tree-vect-patterns.c.jj	2017-01-01 12:45:37.000000000 +0100
+++ gcc/tree-vect-patterns.c	2017-01-31 16:17:48.082197351 +0100
@@ -3158,9 +3158,7 @@  check_bool_pattern (tree var, vec_info *
       break;
 
     CASE_CONVERT:
-      if ((TYPE_PRECISION (TREE_TYPE (rhs1)) != 1
-	   || !TYPE_UNSIGNED (TREE_TYPE (rhs1)))
-	  && TREE_CODE (TREE_TYPE (rhs1)) != BOOLEAN_TYPE)
+      if (!INTEGRAL_BOOLEAN_TYPE_P (TREE_TYPE (rhs1)))
 	return false;
       if (! check_bool_pattern (rhs1, vinfo, stmts))
 	return false;
@@ -3474,9 +3472,7 @@  search_type_for_mask_1 (tree var, vec_in
   if (TREE_CODE (var) != SSA_NAME)
     return NULL_TREE;
 
-  if ((TYPE_PRECISION (TREE_TYPE (var)) != 1
-       || !TYPE_UNSIGNED (TREE_TYPE (var)))
-      && TREE_CODE (TREE_TYPE (var)) != BOOLEAN_TYPE)
+  if (!INTEGRAL_BOOLEAN_TYPE_P (TREE_TYPE (var)))
     return NULL_TREE;
 
   if (!vect_is_simple_use (var, vinfo, &def_stmt, &dt))
@@ -3518,7 +3514,7 @@  search_type_for_mask_1 (tree var, vec_in
 	{
 	  tree comp_vectype, mask_type;
 
-	  if (TREE_CODE (TREE_TYPE (rhs1)) == BOOLEAN_TYPE)
+	  if (INTEGRAL_BOOLEAN_TYPE_P (TREE_TYPE (rhs1)))
 	    {
 	      res = search_type_for_mask_1 (rhs1, vinfo, cache);
 	      res2 = search_type_for_mask_1 (gimple_assign_rhs2 (def_stmt),
@@ -3637,9 +3633,7 @@  vect_recog_bool_pattern (vec<gimple *> *
   var = gimple_assign_rhs1 (last_stmt);
   lhs = gimple_assign_lhs (last_stmt);
 
-  if ((TYPE_PRECISION (TREE_TYPE (var)) != 1
-       || !TYPE_UNSIGNED (TREE_TYPE (var)))
-      && TREE_CODE (TREE_TYPE (var)) != BOOLEAN_TYPE)
+  if (!INTEGRAL_BOOLEAN_TYPE_P (TREE_TYPE (var)))
     return NULL;
 
   hash_set<gimple *> bool_stmts;
@@ -4023,7 +4017,7 @@  vect_recog_mask_conversion_pattern (vec<
 
   /* Now check for binary boolean operations requiring conversion for
      one of operands.  */
-  if (TREE_CODE (TREE_TYPE (lhs)) != BOOLEAN_TYPE)
+  if (!INTEGRAL_BOOLEAN_TYPE_P (TREE_TYPE (lhs)))
     return NULL;
 
   if (rhs_code != BIT_IOR_EXPR
--- gcc/tree-vect-slp.c.jj	2017-01-09 11:35:04.000000000 +0100
+++ gcc/tree-vect-slp.c	2017-01-31 16:17:36.243351080 +0100
@@ -2949,7 +2949,7 @@  vect_get_constant_vectors (tree op, slp_
   gimple_seq ctor_seq = NULL;
 
   /* Check if vector type is a boolean vector.  */
-  if (TREE_CODE (TREE_TYPE (op)) == BOOLEAN_TYPE
+  if (INTEGRAL_BOOLEAN_TYPE_P (TREE_TYPE (op))
       && vect_mask_constant_operand_p (stmt, op_num))
     vector_type
       = build_same_sized_truth_vector_type (STMT_VINFO_VECTYPE (stmt_vinfo));
--- gcc/tree-vect-loop.c.jj	2017-01-05 11:43:11.000000000 +0100
+++ gcc/tree-vect-loop.c	2017-01-31 16:18:17.593814137 +0100
@@ -433,7 +433,7 @@  vect_determine_vectorization_factor (loo
 	      /* Bool ops don't participate in vectorization factor
 		 computation.  For comparison use compared types to
 		 compute a factor.  */
-	      if (TREE_CODE (scalar_type) == BOOLEAN_TYPE
+	      if (INTEGRAL_BOOLEAN_TYPE_P (scalar_type)
 		  && is_gimple_assign (stmt)
 		  && gimple_assign_rhs_code (stmt) != COND_EXPR)
 		{
@@ -445,8 +445,8 @@  vect_determine_vectorization_factor (loo
 		  if (gimple_code (stmt) == GIMPLE_ASSIGN
 		      && TREE_CODE_CLASS (gimple_assign_rhs_code (stmt))
 			 == tcc_comparison
-		      && TREE_CODE (TREE_TYPE (gimple_assign_rhs1 (stmt)))
-			 != BOOLEAN_TYPE)
+		      && !INTEGRAL_BOOLEAN_TYPE_P
+			    (TREE_TYPE (gimple_assign_rhs1 (stmt))))
 		    scalar_type = TREE_TYPE (gimple_assign_rhs1 (stmt));
 		  else
 		    {
@@ -587,7 +587,7 @@  vect_determine_vectorization_factor (loo
 
       if (gimple_code (stmt) == GIMPLE_ASSIGN
 	  && TREE_CODE_CLASS (gimple_assign_rhs_code (stmt)) == tcc_comparison
-	  && TREE_CODE (TREE_TYPE (gimple_assign_rhs1 (stmt))) != BOOLEAN_TYPE)
+	  && !INTEGRAL_BOOLEAN_TYPE_P (TREE_TYPE (gimple_assign_rhs1 (stmt))))
 	{
 	  scalar_type = TREE_TYPE (gimple_assign_rhs1 (stmt));
 	  mask_type = get_mask_type_for_scalar_type (scalar_type);
--- gcc/testsuite/gcc.c-torture/compile/pr79284.c.jj	2017-01-31 16:05:43.103547991 +0100
+++ gcc/testsuite/gcc.c-torture/compile/pr79284.c	2017-01-31 16:05:30.000000000 +0100
@@ -0,0 +1,13 @@ 
+/* PR tree-optimization/79284 */
+
+struct S { unsigned a : 1; } b;
+int c[64];
+
+int
+foo (int x)
+{ 
+  char e, f;
+  for (e = 63; e; e--)
+    f = (c[e] && ~0) != b.a;
+  return f;
+}