* [PATCH] match.pd: Avoid infinite recursion during bswap (int_cst_ovf1) cmp int_cst_ovf2 [PR104644]
@ 2022-02-23 8:32 Jakub Jelinek
2022-02-23 9:36 ` [PATCH] match.pd, v2: " Jakub Jelinek
2022-02-23 10:36 ` [PATCH] match.pd: " Richard Biener
0 siblings, 2 replies; 7+ messages in thread
From: Jakub Jelinek @ 2022-02-23 8:32 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
Hi!
The following patch avoids infinite recursion during generic folding.
The (cmp (bswap @0) INTEGER_CST@1) simplification relies on
(bswap @1) actually being simplified, if it is not simplified, we just
move the bswap from one operand to the other and if @0 is also INTEGER_CST,
we apply the same rule next.
The reason why bswap @1 isn't folded to INTEGER_CST is that the INTEGER_CST
has TREE_OVERFLOW set on it and fold-const-call.cc predicate punts in
such cases:
static inline bool
integer_cst_p (tree t)
{
return TREE_CODE (t) == INTEGER_CST && !TREE_OVERFLOW (t);
}
The patch uses ! modifier to ensure the bswap is simplified, but because
! is only supported in gimple-match, guards it also with #if GIMPLE.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2022-02-22 Jakub Jelinek <jakub@redhat.com>
PR tree-optimization/104644
* match.pd (cmp (bswap @0) INTEGER_CST@1): Restrict optimization to
GIMPLE only and use ! modifier on bswap.
* gcc.dg/pr104644.c: New test.
--- gcc/match.pd.jj 2022-02-18 12:38:06.075393091 +0100
+++ gcc/match.pd 2022-02-22 20:22:02.222022022 +0100
@@ -3959,10 +3959,13 @@ (define_operator_list SYNC_FETCH_AND_AND
(cmp (bswap@2 @0) (bswap @1))
(with { tree ctype = TREE_TYPE (@2); }
(cmp (convert:ctype @0) (convert:ctype @1))))
+#if GIMPLE
(simplify
(cmp (bswap @0) INTEGER_CST@1)
(with { tree ctype = TREE_TYPE (@1); }
- (cmp (convert:ctype @0) (bswap @1)))))
+ (cmp (convert:ctype @0) (bswap! @1))))
+#endif
+ )
/* (bswap(x) >> C1) & C2 can sometimes be simplified to (x >> C3) & C2. */
(simplify
(bit_and (convert1? (rshift@0 (convert2? (bswap@4 @1)) INTEGER_CST@2))
--- gcc/testsuite/gcc.dg/pr104644.c.jj 2022-02-22 20:02:32.020408468 +0100
+++ gcc/testsuite/gcc.dg/pr104644.c 2022-02-22 20:02:04.609785996 +0100
@@ -0,0 +1,9 @@
+/* PR tree-optimization/104644 */
+/* { dg-do compile } */
+/* { dg-options "-Wno-overflow" } */
+
+int
+foo (void)
+{
+ return __builtin_bswap16 (1.31072e+5f) != (signed char) 1.31072e+5f;
+}
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] match.pd, v2: Avoid infinite recursion during bswap (int_cst_ovf1) cmp int_cst_ovf2 [PR104644]
2022-02-23 8:32 [PATCH] match.pd: Avoid infinite recursion during bswap (int_cst_ovf1) cmp int_cst_ovf2 [PR104644] Jakub Jelinek
@ 2022-02-23 9:36 ` Jakub Jelinek
2022-02-23 10:38 ` Richard Biener
2022-02-23 10:36 ` [PATCH] match.pd: " Richard Biener
1 sibling, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2022-02-23 9:36 UTC (permalink / raw)
To: Richard Biener, gcc-patches
On Wed, Feb 23, 2022 at 09:32:41AM +0100, Jakub Jelinek via Gcc-patches wrote:
> The following patch avoids infinite recursion during generic folding.
> The (cmp (bswap @0) INTEGER_CST@1) simplification relies on
> (bswap @1) actually being simplified, if it is not simplified, we just
> move the bswap from one operand to the other and if @0 is also INTEGER_CST,
> we apply the same rule next.
>
> The reason why bswap @1 isn't folded to INTEGER_CST is that the INTEGER_CST
> has TREE_OVERFLOW set on it and fold-const-call.cc predicate punts in
> such cases:
> static inline bool
> integer_cst_p (tree t)
> {
> return TREE_CODE (t) == INTEGER_CST && !TREE_OVERFLOW (t);
> }
> The patch uses ! modifier to ensure the bswap is simplified, but because
> ! is only supported in gimple-match, guards it also with #if GIMPLE.
Here is another variant, which just breaks the possible ping-pong.
If @0 is not INTEGER_CST, we still want to canonicalize to bswap on the
INTEGER_CST (e.g. in the hope that we throw away TREE_OVERFLOW during/after
gimplification), but if it is INTEGER_CST, we don't want to move bswap
to the operand with TREE_OVERFLOW on it.
Ok for trunk if this passes bootstrap/regtest (it fixes the testcase too)?
2022-02-23 Jakub Jelinek <jakub@redhat.com>
PR tree-optimization/104644
* match.pd (cmp (bswap @0) INTEGER_CST@1): Don't simplify
if TREE_OVERFLOW (@1) and @0 is INTEGER_CST.
* gcc.dg/pr104644.c: New test.
--- gcc/match.pd.jj 2022-02-23 09:17:04.867124392 +0100
+++ gcc/match.pd 2022-02-23 10:31:05.417304115 +0100
@@ -3961,8 +3961,9 @@ (define_operator_list SYNC_FETCH_AND_AND
(cmp (convert:ctype @0) (convert:ctype @1))))
(simplify
(cmp (bswap @0) INTEGER_CST@1)
- (with { tree ctype = TREE_TYPE (@1); }
- (cmp (convert:ctype @0) (bswap @1)))))
+ (if (TREE_CODE (@0) != INTEGER_CST || !TREE_OVERFLOW (@1))
+ (with { tree ctype = TREE_TYPE (@1); }
+ (cmp (convert:ctype @0) (bswap @1))))))
/* (bswap(x) >> C1) & C2 can sometimes be simplified to (x >> C3) & C2. */
(simplify
(bit_and (convert1? (rshift@0 (convert2? (bswap@4 @1)) INTEGER_CST@2))
--- gcc/testsuite/gcc.dg/pr104644.c.jj 2022-02-23 10:29:50.704341688 +0100
+++ gcc/testsuite/gcc.dg/pr104644.c 2022-02-23 10:29:50.704341688 +0100
@@ -0,0 +1,9 @@
+/* PR tree-optimization/104644 */
+/* { dg-do compile } */
+/* { dg-options "-Wno-overflow" } */
+
+int
+foo (void)
+{
+ return __builtin_bswap16 (1.31072e+5f) != (signed char) 1.31072e+5f;
+}
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] match.pd: Avoid infinite recursion during bswap (int_cst_ovf1) cmp int_cst_ovf2 [PR104644]
2022-02-23 8:32 [PATCH] match.pd: Avoid infinite recursion during bswap (int_cst_ovf1) cmp int_cst_ovf2 [PR104644] Jakub Jelinek
2022-02-23 9:36 ` [PATCH] match.pd, v2: " Jakub Jelinek
@ 2022-02-23 10:36 ` Richard Biener
2022-02-23 10:39 ` Jakub Jelinek
1 sibling, 1 reply; 7+ messages in thread
From: Richard Biener @ 2022-02-23 10:36 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Wed, 23 Feb 2022, Jakub Jelinek wrote:
> Hi!
>
> The following patch avoids infinite recursion during generic folding.
> The (cmp (bswap @0) INTEGER_CST@1) simplification relies on
> (bswap @1) actually being simplified, if it is not simplified, we just
> move the bswap from one operand to the other and if @0 is also INTEGER_CST,
> we apply the same rule next.
>
> The reason why bswap @1 isn't folded to INTEGER_CST is that the INTEGER_CST
> has TREE_OVERFLOW set on it and fold-const-call.cc predicate punts in
> such cases:
> static inline bool
> integer_cst_p (tree t)
> {
> return TREE_CODE (t) == INTEGER_CST && !TREE_OVERFLOW (t);
> }
> The patch uses ! modifier to ensure the bswap is simplified, but because
> ! is only supported in gimple-match, guards it also with #if GIMPLE.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Does it recurse on GIMPLE as well? If not, the ! is not necessary
since you guard with GIMPLE already (but it deserves a comment I guess).
> 2022-02-22 Jakub Jelinek <jakub@redhat.com>
>
> PR tree-optimization/104644
> * match.pd (cmp (bswap @0) INTEGER_CST@1): Restrict optimization to
> GIMPLE only and use ! modifier on bswap.
>
> * gcc.dg/pr104644.c: New test.
>
> --- gcc/match.pd.jj 2022-02-18 12:38:06.075393091 +0100
> +++ gcc/match.pd 2022-02-22 20:22:02.222022022 +0100
> @@ -3959,10 +3959,13 @@ (define_operator_list SYNC_FETCH_AND_AND
> (cmp (bswap@2 @0) (bswap @1))
> (with { tree ctype = TREE_TYPE (@2); }
> (cmp (convert:ctype @0) (convert:ctype @1))))
> +#if GIMPLE
> (simplify
> (cmp (bswap @0) INTEGER_CST@1)
> (with { tree ctype = TREE_TYPE (@1); }
> - (cmp (convert:ctype @0) (bswap @1)))))
> + (cmp (convert:ctype @0) (bswap! @1))))
> +#endif
> + )
> /* (bswap(x) >> C1) & C2 can sometimes be simplified to (x >> C3) & C2. */
> (simplify
> (bit_and (convert1? (rshift@0 (convert2? (bswap@4 @1)) INTEGER_CST@2))
> --- gcc/testsuite/gcc.dg/pr104644.c.jj 2022-02-22 20:02:32.020408468 +0100
> +++ gcc/testsuite/gcc.dg/pr104644.c 2022-02-22 20:02:04.609785996 +0100
> @@ -0,0 +1,9 @@
> +/* PR tree-optimization/104644 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wno-overflow" } */
> +
> +int
> +foo (void)
> +{
> + return __builtin_bswap16 (1.31072e+5f) != (signed char) 1.31072e+5f;
> +}
>
> Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] match.pd, v2: Avoid infinite recursion during bswap (int_cst_ovf1) cmp int_cst_ovf2 [PR104644]
2022-02-23 9:36 ` [PATCH] match.pd, v2: " Jakub Jelinek
@ 2022-02-23 10:38 ` Richard Biener
2022-02-23 10:51 ` Jakub Jelinek
0 siblings, 1 reply; 7+ messages in thread
From: Richard Biener @ 2022-02-23 10:38 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Wed, 23 Feb 2022, Jakub Jelinek wrote:
> On Wed, Feb 23, 2022 at 09:32:41AM +0100, Jakub Jelinek via Gcc-patches wrote:
> > The following patch avoids infinite recursion during generic folding.
> > The (cmp (bswap @0) INTEGER_CST@1) simplification relies on
> > (bswap @1) actually being simplified, if it is not simplified, we just
> > move the bswap from one operand to the other and if @0 is also INTEGER_CST,
> > we apply the same rule next.
> >
> > The reason why bswap @1 isn't folded to INTEGER_CST is that the INTEGER_CST
> > has TREE_OVERFLOW set on it and fold-const-call.cc predicate punts in
> > such cases:
> > static inline bool
> > integer_cst_p (tree t)
> > {
> > return TREE_CODE (t) == INTEGER_CST && !TREE_OVERFLOW (t);
> > }
> > The patch uses ! modifier to ensure the bswap is simplified, but because
> > ! is only supported in gimple-match, guards it also with #if GIMPLE.
>
> Here is another variant, which just breaks the possible ping-pong.
> If @0 is not INTEGER_CST, we still want to canonicalize to bswap on the
> INTEGER_CST (e.g. in the hope that we throw away TREE_OVERFLOW during/after
> gimplification), but if it is INTEGER_CST, we don't want to move bswap
> to the operand with TREE_OVERFLOW on it.
>
> Ok for trunk if this passes bootstrap/regtest (it fixes the testcase too)?
If we go for a match.pd solution I'd go with the other one but as said
in the PR I think we should constant fold bswap (1(OVF)) but simply
make the (OVF) sticky as done in other constant foldings.
Richard.
> 2022-02-23 Jakub Jelinek <jakub@redhat.com>
>
> PR tree-optimization/104644
> * match.pd (cmp (bswap @0) INTEGER_CST@1): Don't simplify
> if TREE_OVERFLOW (@1) and @0 is INTEGER_CST.
>
> * gcc.dg/pr104644.c: New test.
>
> --- gcc/match.pd.jj 2022-02-23 09:17:04.867124392 +0100
> +++ gcc/match.pd 2022-02-23 10:31:05.417304115 +0100
> @@ -3961,8 +3961,9 @@ (define_operator_list SYNC_FETCH_AND_AND
> (cmp (convert:ctype @0) (convert:ctype @1))))
> (simplify
> (cmp (bswap @0) INTEGER_CST@1)
> - (with { tree ctype = TREE_TYPE (@1); }
> - (cmp (convert:ctype @0) (bswap @1)))))
> + (if (TREE_CODE (@0) != INTEGER_CST || !TREE_OVERFLOW (@1))
> + (with { tree ctype = TREE_TYPE (@1); }
> + (cmp (convert:ctype @0) (bswap @1))))))
> /* (bswap(x) >> C1) & C2 can sometimes be simplified to (x >> C3) & C2. */
> (simplify
> (bit_and (convert1? (rshift@0 (convert2? (bswap@4 @1)) INTEGER_CST@2))
> --- gcc/testsuite/gcc.dg/pr104644.c.jj 2022-02-23 10:29:50.704341688 +0100
> +++ gcc/testsuite/gcc.dg/pr104644.c 2022-02-23 10:29:50.704341688 +0100
> @@ -0,0 +1,9 @@
> +/* PR tree-optimization/104644 */
> +/* { dg-do compile } */
> +/* { dg-options "-Wno-overflow" } */
> +
> +int
> +foo (void)
> +{
> + return __builtin_bswap16 (1.31072e+5f) != (signed char) 1.31072e+5f;
> +}
>
>
> Jakub
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] match.pd: Avoid infinite recursion during bswap (int_cst_ovf1) cmp int_cst_ovf2 [PR104644]
2022-02-23 10:36 ` [PATCH] match.pd: " Richard Biener
@ 2022-02-23 10:39 ` Jakub Jelinek
0 siblings, 0 replies; 7+ messages in thread
From: Jakub Jelinek @ 2022-02-23 10:39 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
On Wed, Feb 23, 2022 at 11:36:26AM +0100, Richard Biener wrote:
> > The following patch avoids infinite recursion during generic folding.
> > The (cmp (bswap @0) INTEGER_CST@1) simplification relies on
> > (bswap @1) actually being simplified, if it is not simplified, we just
> > move the bswap from one operand to the other and if @0 is also INTEGER_CST,
> > we apply the same rule next.
> >
> > The reason why bswap @1 isn't folded to INTEGER_CST is that the INTEGER_CST
> > has TREE_OVERFLOW set on it and fold-const-call.cc predicate punts in
> > such cases:
> > static inline bool
> > integer_cst_p (tree t)
> > {
> > return TREE_CODE (t) == INTEGER_CST && !TREE_OVERFLOW (t);
> > }
> > The patch uses ! modifier to ensure the bswap is simplified, but because
> > ! is only supported in gimple-match, guards it also with #if GIMPLE.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> Does it recurse on GIMPLE as well? If not, the ! is not necessary
> since you guard with GIMPLE already (but it deserves a comment I guess).
It could if TREE_OVERFLOW would somehow appear in there, but given that the
gimplifier and other spots drop_tree_overflow, it is only theoretical.
Note, I've posted a different version of the patch that allows optimizing
this even if GENERIC in most cases. I think I prefer that version.
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] match.pd, v2: Avoid infinite recursion during bswap (int_cst_ovf1) cmp int_cst_ovf2 [PR104644]
2022-02-23 10:38 ` Richard Biener
@ 2022-02-23 10:51 ` Jakub Jelinek
2022-02-23 11:27 ` Richard Biener
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2022-02-23 10:51 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
On Wed, Feb 23, 2022 at 11:38:30AM +0100, Richard Biener wrote:
> If we go for a match.pd solution I'd go with the other one but as said
> in the PR I think we should constant fold bswap (1(OVF)) but simply
> make the (OVF) sticky as done in other constant foldings.
Changing what fold-const-call.cc does at this point seems extremely risky to
me. There are many different builtins, shall we propagate e.g.
TREE_OVERFLOW from INTEGER_CST operands to REAL_CST result, or vice versa,
etc.? I think not folding those is the conservatively right answer, we
don't have time to analyze all those dozens of different builtins and all
their corner cases, and with OVF in there they aren't valid C or C++
constant expressions, so we don't need to fold those during GENERIC,
and in GIMPLE we drop the overflow flags and can fold those there then.
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] match.pd, v2: Avoid infinite recursion during bswap (int_cst_ovf1) cmp int_cst_ovf2 [PR104644]
2022-02-23 10:51 ` Jakub Jelinek
@ 2022-02-23 11:27 ` Richard Biener
0 siblings, 0 replies; 7+ messages in thread
From: Richard Biener @ 2022-02-23 11:27 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, richard.sandiford
On Wed, 23 Feb 2022, Jakub Jelinek wrote:
> On Wed, Feb 23, 2022 at 11:38:30AM +0100, Richard Biener wrote:
> > If we go for a match.pd solution I'd go with the other one but as said
> > in the PR I think we should constant fold bswap (1(OVF)) but simply
> > make the (OVF) sticky as done in other constant foldings.
>
> Changing what fold-const-call.cc does at this point seems extremely risky to
> me. There are many different builtins, shall we propagate e.g.
> TREE_OVERFLOW from INTEGER_CST operands to REAL_CST result, or vice versa,
> etc.? I think not folding those is the conservatively right answer, we
> don't have time to analyze all those dozens of different builtins and all
> their corner cases, and with OVF in there they aren't valid C or C++
> constant expressions, so we don't need to fold those during GENERIC,
> and in GIMPLE we drop the overflow flags and can fold those there then.
Note we're not 100% there on a (OVF)-free GIMPLE IL.
At this point I'd simply adjust
static tree
fold_const_call_1 (combined_fn fn, tree type, tree arg)
{
machine_mode mode = TYPE_MODE (type);
machine_mode arg_mode = TYPE_MODE (TREE_TYPE (arg));
if (integer_cst_p (arg))
{
if (SCALAR_INT_MODE_P (mode))
{
wide_int result;
if (fold_const_call_ss (&result, fn, wi::to_wide (arg),
TYPE_PRECISION (type), TREE_TYPE (arg)))
return wide_int_to_tree (type, result);
to check if (TREE_CODE (arg) == INTEGER_CST) and
return force_fit_type (type, result, 0, TREE_OVERFLOW (arg));
not constant folding something with constant arguments is much
more risky than accidentially dropping a TREE_OVERFLOW (as we can
see with this endless recursion). All cases currently handled
in the fold_const_call_ss overload do not in itself perform
arithmetic that can introduce overflow from an argument that
does not have TREE_OVERFLOW set.
Just fixing this single case leaves others unfixed, but for
example complex_cst_p fails to check for TREE_OVERFLOW already.
Richard.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-02-23 11:28 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-23 8:32 [PATCH] match.pd: Avoid infinite recursion during bswap (int_cst_ovf1) cmp int_cst_ovf2 [PR104644] Jakub Jelinek
2022-02-23 9:36 ` [PATCH] match.pd, v2: " Jakub Jelinek
2022-02-23 10:38 ` Richard Biener
2022-02-23 10:51 ` Jakub Jelinek
2022-02-23 11:27 ` Richard Biener
2022-02-23 10:36 ` [PATCH] match.pd: " Richard Biener
2022-02-23 10:39 ` Jakub Jelinek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).