[17/12] _BitInt a ? ~b : b match.pd fix [PR102989]
Checks
Commit Message
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 <jakub@redhat.com>
PR c/102989
* match.pd (a ? ~b : b): Don't use build_nonstandard_integer_type
for INTEGER_TYPE or BITINT_TYPE.
Jakub
Comments
On Tue, Sep 5, 2023 at 12:28 AM Jakub Jelinek via Gcc-patches
<gcc-patches@gcc.gnu.org> 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 <jakub@redhat.com>
>
> 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.pd 2023-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
>
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
On Tue, Sep 5, 2023 at 2:51 PM Jakub Jelinek <jakub@redhat.com> 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
>
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
@@ -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