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.
