Re: [PATCH] match: Fix `!a?b:c` and `a?~t:t` patterns for signed 1 bit types [PR114666]
On Fri, Apr 12, 2024 at 1:25 AM Andrew Pinski (QUIC) wrote: > > > -Original Message- > > From: Richard Biener > > Sent: Thursday, April 11, 2024 2:31 AM > > To: Andrew Pinski (QUIC) > > Cc: gcc-patches@gcc.gnu.org > > Subject: Re: [PATCH] match: Fix `!a?b:c` and `a?~t:t` patterns for signed 1 > > bit > > types [PR114666] > > > > On Thu, Apr 11, 2024 at 10:43 AM Andrew Pinski > > wrote: > > > > > > The issue here is that the `a?~t:t` pattern assumed (maybe correctly) > > > that a here was always going to be a unsigned boolean type. This fixes > > > the problem in both patterns to cast the operand to boolean type first. > > > > > > I should note that VRP seems to be keep on wanting to produce `a == > > > 0?1:-2` from `((int)a) ^ 1` is a bit odd and partly is the cause of > > > the issue and there seems to be some disconnect on what should be the > > > canonical form. That will be something to look at for GCC 15. > > > > > > Bootstrapped and tested on x86_64-linux-gnu with no regressions. > > > > > > PR tree-optimization/114666 > > > > > > gcc/ChangeLog: > > > > > > * match.pd (`!a?b:c`): Cast `a` to boolean type for cond for > > > gimple. > > > (`a?~t:t`): Cast `a` to boolean type before casting it > > > to the type. > > > > > > gcc/testsuite/ChangeLog: > > > > > > * gcc.c-torture/execute/bitfld-signed1-1.c: New test. > > > > > > Signed-off-by: Andrew Pinski > > > --- > > > gcc/match.pd| 10 +++--- > > > .../gcc.c-torture/execute/bitfld-signed1-1.c| 13 + > > > 2 files changed, 20 insertions(+), 3 deletions(-) create mode 100644 > > > gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c > > > > > > diff --git a/gcc/match.pd b/gcc/match.pd index > > > 15a1e7350d4..ffc928b656a 100644 > > > --- a/gcc/match.pd > > > +++ b/gcc/match.pd > > > @@ -5895,7 +5895,11 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > > > /* !A ? B : C -> A ? C : B. */ > > > (simplify > > >(cnd (logical_inverted_value truth_valued_p@0) @1 @2) > > > - (cnd @0 @2 @1))) > > > + /* For gimple, make sure the operand to COND is a boolean type, > > > + truth_valued_p will match 1bit integers too. */ (if (GIMPLE && > > > + cnd == COND_EXPR) > > > + (cnd (convert:boolean_type_node @0) @2 @1) > > > + (cnd @0 @2 @1 > > > > This looks "wrong" for GENERIC still? > > I tired without the GIMPLE check and ran into the testcase > gcc.dg/torture/builtins-isinf-sign-1.c failing. Because the extra convert was > blocking seeing both sides of an equal was the same (I didn't look into it > further than that). So I decided to limit it to GIMPLE only. > > > But this is not really part of the fix but deciding we should not have > > signed:1 as > > cond operand? I'll note that truth_valued_p allows signed:1. > > > > Maybe as minimal surgery add a TYPE_UNSIGNED (TREE_TPE (@0)) check here > > instead? > > That might work, let me try. > > > > > > /* abs/negative simplifications moved from > > fold_cond_expr_with_comparison. > > > > > > @@ -7099,8 +7103,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) > > > && (!wascmp || TYPE_PRECISION (type) == 1)) > > > (if ((!TYPE_UNSIGNED (type) && TREE_CODE (type) == BOOLEAN_TYPE) > > > || TYPE_PRECISION (type) == 1) > > > -(bit_xor (convert:type @0) @2) > > > -(bit_xor (negate (convert:type @0)) @2) > > > +(bit_xor (convert:type (convert:boolean_type_node @0)) @2) > > > +(bit_xor (negate (convert:type (convert:boolean_type_node @0))) > > > + @2) > > > #endif > > > > This looks OK, but then testing TYPE_UNSIGNED (TREE_TYPE (@0)) might be > > better? > > > > Let me do that just like the other pattern. > > > Does this all just go downhill from what VRP creates? That is, would IL > > checking have had a chance detecting it if we say signed:1 are not valid as > > condition? > > Yes. So what VRP produces in the testcase is: > `_2 == 0 ? 1 : -2u` (where _2 is the signed 1bit integer). > Now maybe the COND_EXPR should be the canonical form for constants (but that > is for a different patch I think, I added it to the list of things I should > look into for GCC 15). Ah OK, so the !A ? B : C -> A ? C : B transform turns the "proper" conditional into an improper one (if we want to restrict it). And then the other pattern matches doing the wrong transform. > > > > That said, the latter pattern definitely needs guarding/adjustment, I'm not > > sure the former is wrong? Semantically [VEC_]COND_EXPR is op0 != 0 ? ... : > > ... > > I forgot to mention that to fix the bug only one of the 2 hunks are needed. > > > > > Richard. > > > > > /* Simplify pointer equality compares using PTA. */ diff --git > > > a/gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c > > > b/gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c > > > new file mode 100644 > > > index 000..b0ff120ea51 > > > --- /dev/null > > > +++
RE: [PATCH] match: Fix `!a?b:c` and `a?~t:t` patterns for signed 1 bit types [PR114666]
> -Original Message-
> From: Richard Biener
> Sent: Thursday, April 11, 2024 2:31 AM
> To: Andrew Pinski (QUIC)
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] match: Fix `!a?b:c` and `a?~t:t` patterns for signed 1
> bit
> types [PR114666]
>
> On Thu, Apr 11, 2024 at 10:43 AM Andrew Pinski
> wrote:
> >
> > The issue here is that the `a?~t:t` pattern assumed (maybe correctly)
> > that a here was always going to be a unsigned boolean type. This fixes
> > the problem in both patterns to cast the operand to boolean type first.
> >
> > I should note that VRP seems to be keep on wanting to produce `a ==
> > 0?1:-2` from `((int)a) ^ 1` is a bit odd and partly is the cause of
> > the issue and there seems to be some disconnect on what should be the
> > canonical form. That will be something to look at for GCC 15.
> >
> > Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> >
> > PR tree-optimization/114666
> >
> > gcc/ChangeLog:
> >
> > * match.pd (`!a?b:c`): Cast `a` to boolean type for cond for
> > gimple.
> > (`a?~t:t`): Cast `a` to boolean type before casting it
> > to the type.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.c-torture/execute/bitfld-signed1-1.c: New test.
> >
> > Signed-off-by: Andrew Pinski
> > ---
> > gcc/match.pd| 10 +++---
> > .../gcc.c-torture/execute/bitfld-signed1-1.c| 13 +
> > 2 files changed, 20 insertions(+), 3 deletions(-) create mode 100644
> > gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
> >
> > diff --git a/gcc/match.pd b/gcc/match.pd index
> > 15a1e7350d4..ffc928b656a 100644
> > --- a/gcc/match.pd
> > +++ b/gcc/match.pd
> > @@ -5895,7 +5895,11 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> > /* !A ? B : C -> A ? C : B. */
> > (simplify
> >(cnd (logical_inverted_value truth_valued_p@0) @1 @2)
> > - (cnd @0 @2 @1)))
> > + /* For gimple, make sure the operand to COND is a boolean type,
> > + truth_valued_p will match 1bit integers too. */ (if (GIMPLE &&
> > + cnd == COND_EXPR)
> > + (cnd (convert:boolean_type_node @0) @2 @1)
> > + (cnd @0 @2 @1
>
> This looks "wrong" for GENERIC still?
I tired without the GIMPLE check and ran into the testcase
gcc.dg/torture/builtins-isinf-sign-1.c failing. Because the extra convert was
blocking seeing both sides of an equal was the same (I didn't look into it
further than that). So I decided to limit it to GIMPLE only.
> But this is not really part of the fix but deciding we should not have
> signed:1 as
> cond operand? I'll note that truth_valued_p allows signed:1.
>
> Maybe as minimal surgery add a TYPE_UNSIGNED (TREE_TPE (@0)) check here
> instead?
That might work, let me try.
>
> > /* abs/negative simplifications moved from
> fold_cond_expr_with_comparison.
> >
> > @@ -7099,8 +7103,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> > && (!wascmp || TYPE_PRECISION (type) == 1))
> > (if ((!TYPE_UNSIGNED (type) && TREE_CODE (type) == BOOLEAN_TYPE)
> > || TYPE_PRECISION (type) == 1)
> > -(bit_xor (convert:type @0) @2)
> > -(bit_xor (negate (convert:type @0)) @2)
> > +(bit_xor (convert:type (convert:boolean_type_node @0)) @2)
> > +(bit_xor (negate (convert:type (convert:boolean_type_node @0)))
> > + @2)
> > #endif
>
> This looks OK, but then testing TYPE_UNSIGNED (TREE_TYPE (@0)) might be
> better?
>
Let me do that just like the other pattern.
> Does this all just go downhill from what VRP creates? That is, would IL
> checking have had a chance detecting it if we say signed:1 are not valid as
> condition?
Yes. So what VRP produces in the testcase is:
`_2 == 0 ? 1 : -2u` (where _2 is the signed 1bit integer).
Now maybe the COND_EXPR should be the canonical form for constants (but that is
for a different patch I think, I added it to the list of things I should look
into for GCC 15).
>
> That said, the latter pattern definitely needs guarding/adjustment, I'm not
> sure the former is wrong? Semantically [VEC_]COND_EXPR is op0 != 0 ? ... :
> ...
I forgot to mention that to fix the bug only one of the 2 hunks are needed.
>
> Richard.
>
> > /* Simplify pointer equality compares using PTA. */ diff --git
> > a/gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
> > b/gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
> > new file mode 100644
> > index 000..b0ff120ea51
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
> > @@ -0,0 +1,13 @@
> > +/* PR tree-optimization/114666 */
> > +/* We used to miscompile this to be always aborting
> > + due to the use of the signed 1bit into the COND_EXPR. */
> > +
> > +struct {
> > + signed a : 1;
> > +} b = {-1};
> > +char c;
> > +int main()
> > +{
> > + if ((b.a ^ 1UL) < 3)
> > +__builtin_abort();
> > +}
> > --
> > 2.43.0
> >
Re: [PATCH] match: Fix `!a?b:c` and `a?~t:t` patterns for signed 1 bit types [PR114666]
On Thu, Apr 11, 2024 at 10:43 AM Andrew Pinski wrote:
>
> The issue here is that the `a?~t:t` pattern assumed (maybe correctly) that a
> here was always going to be a unsigned boolean type. This fixes the problem
> in both patterns to cast the operand to boolean type first.
>
> I should note that VRP seems to be keep on wanting to produce `a == 0?1:-2`
> from `((int)a) ^ 1` is a bit odd and partly is the cause of the issue and
> there
> seems to be some disconnect on what should be the canonical form. That will be
> something to look at for GCC 15.
>
> Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> PR tree-optimization/114666
>
> gcc/ChangeLog:
>
> * match.pd (`!a?b:c`): Cast `a` to boolean type for cond for
> gimple.
> (`a?~t:t`): Cast `a` to boolean type before casting it
> to the type.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.c-torture/execute/bitfld-signed1-1.c: New test.
>
> Signed-off-by: Andrew Pinski
> ---
> gcc/match.pd| 10 +++---
> .../gcc.c-torture/execute/bitfld-signed1-1.c| 13 +
> 2 files changed, 20 insertions(+), 3 deletions(-)
> create mode 100644 gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
>
> diff --git a/gcc/match.pd b/gcc/match.pd
> index 15a1e7350d4..ffc928b656a 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -5895,7 +5895,11 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> /* !A ? B : C -> A ? C : B. */
> (simplify
>(cnd (logical_inverted_value truth_valued_p@0) @1 @2)
> - (cnd @0 @2 @1)))
> + /* For gimple, make sure the operand to COND is a boolean type,
> + truth_valued_p will match 1bit integers too. */
> + (if (GIMPLE && cnd == COND_EXPR)
> + (cnd (convert:boolean_type_node @0) @2 @1)
> + (cnd @0 @2 @1
This looks "wrong" for GENERIC still?
But this is not really part of the fix but deciding we should not have
signed:1 as
cond operand? I'll note that truth_valued_p allows signed:1.
Maybe as minimal surgery add a TYPE_UNSIGNED (TREE_TPE (@0)) check here
instead?
> /* abs/negative simplifications moved from fold_cond_expr_with_comparison.
>
> @@ -7099,8 +7103,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> && (!wascmp || TYPE_PRECISION (type) == 1))
> (if ((!TYPE_UNSIGNED (type) && TREE_CODE (type) == BOOLEAN_TYPE)
> || TYPE_PRECISION (type) == 1)
> -(bit_xor (convert:type @0) @2)
> -(bit_xor (negate (convert:type @0)) @2)
> +(bit_xor (convert:type (convert:boolean_type_node @0)) @2)
> +(bit_xor (negate (convert:type (convert:boolean_type_node @0))) @2)
> #endif
This looks OK, but then testing TYPE_UNSIGNED (TREE_TYPE (@0)) might be
better?
Does this all just go downhill from what VRP creates? That is, would
IL checking
have had a chance detecting it if we say signed:1 are not valid as condition?
That said, the latter pattern definitely needs guarding/adjustment, I'm not sure
the former is wrong? Semantically [VEC_]COND_EXPR is op0 != 0 ? ... : ...
Richard.
> /* Simplify pointer equality compares using PTA. */
> diff --git a/gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
> b/gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
> new file mode 100644
> index 000..b0ff120ea51
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
> @@ -0,0 +1,13 @@
> +/* PR tree-optimization/114666 */
> +/* We used to miscompile this to be always aborting
> + due to the use of the signed 1bit into the COND_EXPR. */
> +
> +struct {
> + signed a : 1;
> +} b = {-1};
> +char c;
> +int main()
> +{
> + if ((b.a ^ 1UL) < 3)
> +__builtin_abort();
> +}
> --
> 2.43.0
>
[PATCH] match: Fix `!a?b:c` and `a?~t:t` patterns for signed 1 bit types [PR114666]
The issue here is that the `a?~t:t` pattern assumed (maybe correctly) that a
here was always going to be a unsigned boolean type. This fixes the problem
in both patterns to cast the operand to boolean type first.
I should note that VRP seems to be keep on wanting to produce `a == 0?1:-2`
from `((int)a) ^ 1` is a bit odd and partly is the cause of the issue and there
seems to be some disconnect on what should be the canonical form. That will be
something to look at for GCC 15.
Bootstrapped and tested on x86_64-linux-gnu with no regressions.
PR tree-optimization/114666
gcc/ChangeLog:
* match.pd (`!a?b:c`): Cast `a` to boolean type for cond for
gimple.
(`a?~t:t`): Cast `a` to boolean type before casting it
to the type.
gcc/testsuite/ChangeLog:
* gcc.c-torture/execute/bitfld-signed1-1.c: New test.
Signed-off-by: Andrew Pinski
---
gcc/match.pd| 10 +++---
.../gcc.c-torture/execute/bitfld-signed1-1.c| 13 +
2 files changed, 20 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
diff --git a/gcc/match.pd b/gcc/match.pd
index 15a1e7350d4..ffc928b656a 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -5895,7 +5895,11 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
/* !A ? B : C -> A ? C : B. */
(simplify
(cnd (logical_inverted_value truth_valued_p@0) @1 @2)
- (cnd @0 @2 @1)))
+ /* For gimple, make sure the operand to COND is a boolean type,
+ truth_valued_p will match 1bit integers too. */
+ (if (GIMPLE && cnd == COND_EXPR)
+ (cnd (convert:boolean_type_node @0) @2 @1)
+ (cnd @0 @2 @1
/* abs/negative simplifications moved from fold_cond_expr_with_comparison.
@@ -7099,8 +7103,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
&& (!wascmp || TYPE_PRECISION (type) == 1))
(if ((!TYPE_UNSIGNED (type) && TREE_CODE (type) == BOOLEAN_TYPE)
|| TYPE_PRECISION (type) == 1)
-(bit_xor (convert:type @0) @2)
-(bit_xor (negate (convert:type @0)) @2)
+(bit_xor (convert:type (convert:boolean_type_node @0)) @2)
+(bit_xor (negate (convert:type (convert:boolean_type_node @0))) @2)
#endif
/* Simplify pointer equality compares using PTA. */
diff --git a/gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
b/gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
new file mode 100644
index 000..b0ff120ea51
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/bitfld-signed1-1.c
@@ -0,0 +1,13 @@
+/* PR tree-optimization/114666 */
+/* We used to miscompile this to be always aborting
+ due to the use of the signed 1bit into the COND_EXPR. */
+
+struct {
+ signed a : 1;
+} b = {-1};
+char c;
+int main()
+{
+ if ((b.a ^ 1UL) < 3)
+__builtin_abort();
+}
--
2.43.0
