Re: [PATCH 17/12] _BitInt a ? ~b : b match.pd fix [PR102989]
On Tue, Sep 05, 2023 at 03:07:15PM -0700, Andrew Pinski wrote: > Note I notice another all to build_nonstandard_integer_type in this > match pattern which might also need to be fixed: > /* For (x << c) >> c, optimize into x & ((unsigned)-1 >> c) for >unsigned x OR truncate into the precision(type) - c lowest bits >of signed x (if they have mode precision or a precision of 1). */ > (simplify > (rshift (nop_convert? (lshift @0 INTEGER_CST@1)) @@1) > (if (wi::ltu_p (wi::to_wide (@1), element_precision (type))) > (if (TYPE_UNSIGNED (type)) >(bit_and (convert @0) (rshift { build_minus_one_cst (type); } @1)) >(if (INTEGRAL_TYPE_P (type)) > (with { > int width = element_precision (type) - tree_to_uhwi (@1); > tree stype = build_nonstandard_integer_type (width, 0); > } > (if (width == 1 || type_has_mode_precision_p (stype)) > (convert (convert:stype @0 > > Do we have ranges on BITINT_TYPEs? If so the two_value_replacement > pattern in match.pd has a similar issue too. > (that is where I copied the code to use build_nonstandard_integer_type > from originally too. BITINT_TYPEs do have ranges like any other integral types. But the larger ones should be lowered shortly after IPA and arithmetics etc. on them shouldn't appear in later passes. Most BITINT_TYPEs or for the above cases overly large INTEGER_TYPEs will not satisfy type_has_mode_precision_p (but it isn't a good idea to create such types only to throw them away). But some BITINT_TYPEs do have non-BLKmode TYPE_MODE, they follow what modes get similarly sized structures, so on some targets one could get OImode or XImode. For the above case, I think we definitely don't want to emit integral types with such modes because nothing during expansion will support them. So, I wonder if the above shouldn't do tree stype = NULL_TREE; if (width <= (targetm.scalar_mode_supported_p (TImode) ? TImode : DImode)) stype = build_nonstandard_integer_type (width, 0); and || (stype && type_has_mode_precision_p (stype))) so that we don't create types which wouldn't work. Jakub
Re: [PATCH 17/12] _BitInt a ? ~b : b match.pd fix [PR102989]
On Tue, Sep 5, 2023 at 2:51 PM Jakub Jelinek wrote: > > On Tue, Sep 05, 2023 at 02:27:10PM -0700, Andrew Pinski wrote: > > > I admit it isn't really clear to me what do you want to achieve by the > > > above build_nonstandard_integer_type. Is it because of BOOLEAN_TYPE > > > or perhaps ENUMERAL_TYPE as well? > > > > Yes I was worried about types where the precision was set but MIN/MAX > > of that type was not over the full precision and would not include > > both 0 and allones in that range. > > There is another match.pd pattern where we do a similar thing with > > calling build_nonstandard_integer_type for a similar reason but > > because we don't know if the type includes 0, 1, and allones in their > > range. > > Ah, in that case you should use range_check_type, that is used already > in multiple spots in match.pd for the same purpose. It can return NULL and > in that case one should punt on the optimization. Otherwise, that is the > function which ensures that the type is unsigned and max + 1 is min and min > - 1 is max. > And for me, I should add BITINT_TYPE handling to that function. Hmm maybe range_check_type is the correct one here. > > > > If type is INTEGER_TYPE or BITINT_TYPE, one doesn't really need to create > > > a > > > new type, type already is an integral type with that precision and > > > signedness. In other places using unsigned_type_for or signed_type_for > > > might be better than using build_nonstandard_integer_type if that is what > > > one wants to achieve, those functions handle BITINT_TYPE. > > > > Maybe here we should just use `signed_or_unsigned_type_for (type, > > TYPE_SIGN (type));` > > instead of build_nonstandard_integer_type. > > No, signed_or_unsigned_type_for (TYPE_UNSIGNED (type), type) will just return > type. > if (ANY_INTEGRAL_TYPE_P (type) && TYPE_UNSIGNED (type) == unsignedp) > return type; Oh I missed that. Note I notice another all to build_nonstandard_integer_type in this match pattern which might also need to be fixed: /* For (x << c) >> c, optimize into x & ((unsigned)-1 >> c) for unsigned x OR truncate into the precision(type) - c lowest bits of signed x (if they have mode precision or a precision of 1). */ (simplify (rshift (nop_convert? (lshift @0 INTEGER_CST@1)) @@1) (if (wi::ltu_p (wi::to_wide (@1), element_precision (type))) (if (TYPE_UNSIGNED (type)) (bit_and (convert @0) (rshift { build_minus_one_cst (type); } @1)) (if (INTEGRAL_TYPE_P (type)) (with { int width = element_precision (type) - tree_to_uhwi (@1); tree stype = build_nonstandard_integer_type (width, 0); } (if (width == 1 || type_has_mode_precision_p (stype)) (convert (convert:stype @0 Do we have ranges on BITINT_TYPEs? If so the two_value_replacement pattern in match.pd has a similar issue too. (that is where I copied the code to use build_nonstandard_integer_type from originally too. Thanks, Andrew > > Jakub >
Re: [PATCH 17/12] _BitInt a ? ~b : b match.pd fix [PR102989]
On Tue, Sep 05, 2023 at 02:27:10PM -0700, Andrew Pinski wrote: > > I admit it isn't really clear to me what do you want to achieve by the > > above build_nonstandard_integer_type. Is it because of BOOLEAN_TYPE > > or perhaps ENUMERAL_TYPE as well? > > Yes I was worried about types where the precision was set but MIN/MAX > of that type was not over the full precision and would not include > both 0 and allones in that range. > There is another match.pd pattern where we do a similar thing with > calling build_nonstandard_integer_type for a similar reason but > because we don't know if the type includes 0, 1, and allones in their > range. Ah, in that case you should use range_check_type, that is used already in multiple spots in match.pd for the same purpose. It can return NULL and in that case one should punt on the optimization. Otherwise, that is the function which ensures that the type is unsigned and max + 1 is min and min - 1 is max. And for me, I should add BITINT_TYPE handling to that function. > > If type is INTEGER_TYPE or BITINT_TYPE, one doesn't really need to create a > > new type, type already is an integral type with that precision and > > signedness. In other places using unsigned_type_for or signed_type_for > > might be better than using build_nonstandard_integer_type if that is what > > one wants to achieve, those functions handle BITINT_TYPE. > > Maybe here we should just use `signed_or_unsigned_type_for (type, > TYPE_SIGN (type));` > instead of build_nonstandard_integer_type. No, signed_or_unsigned_type_for (TYPE_UNSIGNED (type), type) will just return type. if (ANY_INTEGRAL_TYPE_P (type) && TYPE_UNSIGNED (type) == unsignedp) return type; Jakub
Re: [PATCH 17/12] _BitInt a ? ~b : b match.pd fix [PR102989]
On Tue, Sep 5, 2023 at 12:28 AM Jakub Jelinek via Gcc-patches wrote: > > On Wed, Aug 09, 2023 at 12:19:54PM -0700, Andrew Pinski via Gcc-patches wrote: > > PR tree-optimization/110937 > > PR tree-optimization/100798 > > --- a/gcc/match.pd > > +++ b/gcc/match.pd > > @@ -6460,6 +6460,20 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > >(if (cmp == NE_EXPR) > > { constant_boolean_node (true, type); }))) > > > > +#if GIMPLE > > +/* a?~t:t -> (-(a))^t */ > > +(simplify > > + (cond @0 @1 @2) > > + (if (INTEGRAL_TYPE_P (type) > > + && bitwise_inverted_equal_p (@1, @2)) > > + (with { > > +auto prec = TYPE_PRECISION (type); > > +auto unsign = TYPE_UNSIGNED (type); > > +tree inttype = build_nonstandard_integer_type (prec, unsign); > > + } > > + (convert (bit_xor (negate (convert:inttype @0)) (convert:inttype > > @2)) > > +#endif > > This broke one bitint test - bitint-42.c for -O1 and -Os (in admittedly not > yet > committed series). > Using build_nonstandard_integer_type this way doesn't work well for larger > precision BITINT_TYPEs, because it always creates an INTEGER_TYPE and > say 467-bit INTEGER_TYPE doesn't work very well. To get a BITINT_TYPE, one > needs to use build_bitint_type instead (but similarly to > build_nonstandard_integer_type one should first make sure such a type > actually can be created). > > I admit it isn't really clear to me what do you want to achieve by the > above build_nonstandard_integer_type. Is it because of BOOLEAN_TYPE > or perhaps ENUMERAL_TYPE as well? Yes I was worried about types where the precision was set but MIN/MAX of that type was not over the full precision and would not include both 0 and allones in that range. There is another match.pd pattern where we do a similar thing with calling build_nonstandard_integer_type for a similar reason but because we don't know if the type includes 0, 1, and allones in their range. > > If type is INTEGER_TYPE or BITINT_TYPE, one doesn't really need to create a > new type, type already is an integral type with that precision and > signedness. In other places using unsigned_type_for or signed_type_for > might be better than using build_nonstandard_integer_type if that is what > one wants to achieve, those functions handle BITINT_TYPE. Maybe here we should just use `signed_or_unsigned_type_for (type, TYPE_SIGN (type));` instead of build_nonstandard_integer_type. Thanks, Andrew > > Or shall we instead test for == BOOLEAN_TYPE (or if ENUMERAL_TYPE for > some reason needs the same treatment also || == ENUMERAL_TYPE)? > > 2023-09-05 Jakub Jelinek > > PR c/102989 > * match.pd (a ? ~b : b): Don't use build_nonstandard_integer_type > for INTEGER_TYPE or BITINT_TYPE. > > --- gcc/match.pd.jj 2023-09-04 09:45:33.553058301 +0200 > +++ gcc/match.pd2023-09-05 08:45:53.258078971 +0200 > @@ -6631,7 +6631,9 @@ (define_operator_list SYNC_FETCH_AND_AND > (with { > auto prec = TYPE_PRECISION (type); > auto unsign = TYPE_UNSIGNED (type); > - tree inttype = build_nonstandard_integer_type (prec, unsign); > + tree inttype = type; > + if (TREE_CODE (type) != INTEGER_TYPE && TREE_CODE (type) != BITINT_TYPE) > + inttype = build_nonstandard_integer_type (prec, unsign); > } > (convert (bit_xor (negate (convert:inttype @0)) (convert:inttype > @2))) > #endif > > > Jakub >
[PATCH 17/12] _BitInt a ? ~b : b match.pd fix [PR102989]
On Wed, Aug 09, 2023 at 12:19:54PM -0700, Andrew Pinski via Gcc-patches wrote: > PR tree-optimization/110937 > PR tree-optimization/100798 > --- a/gcc/match.pd > +++ b/gcc/match.pd > @@ -6460,6 +6460,20 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) >(if (cmp == NE_EXPR) > { constant_boolean_node (true, type); }))) > > +#if GIMPLE > +/* a?~t:t -> (-(a))^t */ > +(simplify > + (cond @0 @1 @2) > + (if (INTEGRAL_TYPE_P (type) > + && bitwise_inverted_equal_p (@1, @2)) > + (with { > +auto prec = TYPE_PRECISION (type); > +auto unsign = TYPE_UNSIGNED (type); > +tree inttype = build_nonstandard_integer_type (prec, unsign); > + } > + (convert (bit_xor (negate (convert:inttype @0)) (convert:inttype @2)) > +#endif This broke one bitint test - bitint-42.c for -O1 and -Os (in admittedly not yet committed series). Using build_nonstandard_integer_type this way doesn't work well for larger precision BITINT_TYPEs, because it always creates an INTEGER_TYPE and say 467-bit INTEGER_TYPE doesn't work very well. To get a BITINT_TYPE, one needs to use build_bitint_type instead (but similarly to build_nonstandard_integer_type one should first make sure such a type actually can be created). I admit it isn't really clear to me what do you want to achieve by the above build_nonstandard_integer_type. Is it because of BOOLEAN_TYPE or perhaps ENUMERAL_TYPE as well? If type is INTEGER_TYPE or BITINT_TYPE, one doesn't really need to create a new type, type already is an integral type with that precision and signedness. In other places using unsigned_type_for or signed_type_for might be better than using build_nonstandard_integer_type if that is what one wants to achieve, those functions handle BITINT_TYPE. Or shall we instead test for == BOOLEAN_TYPE (or if ENUMERAL_TYPE for some reason needs the same treatment also || == ENUMERAL_TYPE)? 2023-09-05 Jakub Jelinek PR c/102989 * match.pd (a ? ~b : b): Don't use build_nonstandard_integer_type for INTEGER_TYPE or BITINT_TYPE. --- gcc/match.pd.jj 2023-09-04 09:45:33.553058301 +0200 +++ gcc/match.pd2023-09-05 08:45:53.258078971 +0200 @@ -6631,7 +6631,9 @@ (define_operator_list SYNC_FETCH_AND_AND (with { auto prec = TYPE_PRECISION (type); auto unsign = TYPE_UNSIGNED (type); - tree inttype = build_nonstandard_integer_type (prec, unsign); + tree inttype = type; + if (TREE_CODE (type) != INTEGER_TYPE && TREE_CODE (type) != BITINT_TYPE) + inttype = build_nonstandard_integer_type (prec, unsign); } (convert (bit_xor (negate (convert:inttype @0)) (convert:inttype @2))) #endif Jakub