diff mbox series

Fix PR 110954: wrong code with cmp | !cmp

Message ID 20230810001956.2680884-1-apinski@marvell.com
State Unresolved
Headers
Series Fix PR 110954: wrong code with cmp | !cmp |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Andrew Pinski Aug. 10, 2023, 12:19 a.m. UTC 
  This was an oversight on my part not realizing that
comparisons in generic can have a non-boolean type.
This means if we have `(f < 0) | !(f < 0)` we would
optimize this to -1 rather than just 1.
This patch just adds the check for the type of the comparisons
to be boolean type to keep the optimization in that case.

OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.

	PR 110954

gcc/ChangeLog:

	* generic-match-head.cc (bitwise_inverted_equal_p): Check
	the type of the comparison to be boolean too.

gcc/testsuite/ChangeLog:

	* gcc.c-torture/execute/pr110954-1.c: New test.
---
 gcc/generic-match-head.cc                        |  3 ++-
 gcc/testsuite/gcc.c-torture/execute/pr110954-1.c | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110954-1.c

Comments

Richard Biener Aug. 10, 2023, 7:30 a.m. UTC | #1 
On Thu, Aug 10, 2023 at 2:21 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> This was an oversight on my part not realizing that
> comparisons in generic can have a non-boolean type.
> This means if we have `(f < 0) | !(f < 0)` we would
> optimize this to -1 rather than just 1.
> This patch just adds the check for the type of the comparisons
> to be boolean type to keep the optimization in that case.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
>         PR 110954
>
> gcc/ChangeLog:
>
>         * generic-match-head.cc (bitwise_inverted_equal_p): Check
>         the type of the comparison to be boolean too.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.c-torture/execute/pr110954-1.c: New test.
> ---
>  gcc/generic-match-head.cc                        |  3 ++-
>  gcc/testsuite/gcc.c-torture/execute/pr110954-1.c | 10 ++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110954-1.c
>
> diff --git a/gcc/generic-match-head.cc b/gcc/generic-match-head.cc
> index ddaf22f2179..ac2119bfdd0 100644
> --- a/gcc/generic-match-head.cc
> +++ b/gcc/generic-match-head.cc
> @@ -146,7 +146,8 @@ bitwise_inverted_equal_p (tree expr1, tree expr2)
>        && bitwise_equal_p (expr1, TREE_OPERAND (expr2, 0)))
>      return true;
>    if (COMPARISON_CLASS_P (expr1)
> -      && COMPARISON_CLASS_P (expr2))
> +      && COMPARISON_CLASS_P (expr2)
> +      && TREE_CODE (TREE_TYPE (expr1)) == BOOLEAN_TYPE)

in other places we restrict this to single-bit integral types instead which
covers a few more cases and also would handle BOOLEAN_TYPE
with either padding or non-padding extra bits correctly (IIRC fortran
has only padding bits but Ada has BOOLEAN_TYPEs with possibly
> 1 bit precision and arbitrary signedness - maybe even with custom
true/false values).

Richard.

>      {
>        tree op10 = TREE_OPERAND (expr1, 0);
>        tree op20 = TREE_OPERAND (expr2, 0);
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr110954-1.c b/gcc/testsuite/gcc.c-torture/execute/pr110954-1.c
> new file mode 100644
> index 00000000000..8aad758e10f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr110954-1.c
> @@ -0,0 +1,10 @@
> +
> +#define comparison (f < 0)
> +int main() {
> +  int f = 0;
> +  int d = comparison | !comparison;
> +  if (d != 1)
> +    __builtin_abort();
> +  return 0;
> +}
> +
> --
> 2.31.1
>
Andrew Pinski Aug. 10, 2023, 6:58 p.m. UTC | #2 
On Thu, Aug 10, 2023 at 12:32 AM Richard Biener via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Thu, Aug 10, 2023 at 2:21 AM Andrew Pinski via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > This was an oversight on my part not realizing that
> > comparisons in generic can have a non-boolean type.
> > This means if we have `(f < 0) | !(f < 0)` we would
> > optimize this to -1 rather than just 1.
> > This patch just adds the check for the type of the comparisons
> > to be boolean type to keep the optimization in that case.
> >
> > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> >
> >         PR 110954
> >
> > gcc/ChangeLog:
> >
> >         * generic-match-head.cc (bitwise_inverted_equal_p): Check
> >         the type of the comparison to be boolean too.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * gcc.c-torture/execute/pr110954-1.c: New test.
> > ---
> >  gcc/generic-match-head.cc                        |  3 ++-
> >  gcc/testsuite/gcc.c-torture/execute/pr110954-1.c | 10 ++++++++++
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> >  create mode 100644 gcc/testsuite/gcc.c-torture/execute/pr110954-1.c
> >
> > diff --git a/gcc/generic-match-head.cc b/gcc/generic-match-head.cc
> > index ddaf22f2179..ac2119bfdd0 100644
> > --- a/gcc/generic-match-head.cc
> > +++ b/gcc/generic-match-head.cc
> > @@ -146,7 +146,8 @@ bitwise_inverted_equal_p (tree expr1, tree expr2)
> >        && bitwise_equal_p (expr1, TREE_OPERAND (expr2, 0)))
> >      return true;
> >    if (COMPARISON_CLASS_P (expr1)
> > -      && COMPARISON_CLASS_P (expr2))
> > +      && COMPARISON_CLASS_P (expr2)
> > +      && TREE_CODE (TREE_TYPE (expr1)) == BOOLEAN_TYPE)
>
> in other places we restrict this to single-bit integral types instead which
> covers a few more cases and also would handle BOOLEAN_TYPE
> with either padding or non-padding extra bits correctly (IIRC fortran
> has only padding bits but Ada has BOOLEAN_TYPEs with possibly
> > 1 bit precision and arbitrary signedness - maybe even with custom
> true/false values).

Yes and I have a better patch which still allows us to optimize the
case of `cmp & ~cmp` and `cmp | ~cmp`. I will post it after it
finishes bootstrapping.

Thanks,
Andrew

>
> Richard.
>
> >      {
> >        tree op10 = TREE_OPERAND (expr1, 0);
> >        tree op20 = TREE_OPERAND (expr2, 0);
> > diff --git a/gcc/testsuite/gcc.c-torture/execute/pr110954-1.c b/gcc/testsuite/gcc.c-torture/execute/pr110954-1.c
> > new file mode 100644
> > index 00000000000..8aad758e10f
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.c-torture/execute/pr110954-1.c
> > @@ -0,0 +1,10 @@
> > +
> > +#define comparison (f < 0)
> > +int main() {
> > +  int f = 0;
> > +  int d = comparison | !comparison;
> > +  if (d != 1)
> > +    __builtin_abort();
> > +  return 0;
> > +}
> > +
> > --
> > 2.31.1
> >
diff mbox series

Patch

diff --git a/gcc/generic-match-head.cc b/gcc/generic-match-head.cc
index ddaf22f2179..ac2119bfdd0 100644
--- a/gcc/generic-match-head.cc
+++ b/gcc/generic-match-head.cc
@@ -146,7 +146,8 @@  bitwise_inverted_equal_p (tree expr1, tree expr2)
       && bitwise_equal_p (expr1, TREE_OPERAND (expr2, 0)))
     return true;
   if (COMPARISON_CLASS_P (expr1)
-      && COMPARISON_CLASS_P (expr2))
+      && COMPARISON_CLASS_P (expr2)
+      && TREE_CODE (TREE_TYPE (expr1)) == BOOLEAN_TYPE)
     {
       tree op10 = TREE_OPERAND (expr1, 0);
       tree op20 = TREE_OPERAND (expr2, 0);
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr110954-1.c b/gcc/testsuite/gcc.c-torture/execute/pr110954-1.c
new file mode 100644
index 00000000000..8aad758e10f
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr110954-1.c
@@ -0,0 +1,10 @@ 
+
+#define comparison (f < 0)
+int main() {
+  int f = 0;
+  int d = comparison | !comparison;
+  if (d != 1)
+    __builtin_abort();
+  return 0;
+}
+