[PATCH] tree-optimization: [PR102622]: wrong code due to signed one bit integer and "a?-1:0"

[PATCH] tree-optimization: [PR102622]: wrong code due to signed one bit integer and "a?-1:0"

[apinski]

From: Andrew Pinski <apinski@marvell.com>

So it turns out this is kinda of a latent bug but not really latent.
In GCC 9 and 10, phi-opt would transform a?-1:0 (even for signed 1-bit integer)
to -(type)a but the type is an one bit integer which means the negation is
undefined. GCC 11 fixed the problem by checking for a?pow2cst:0 transformation
before a?-1:0 transformation.

When I added the transformations to match.pd, I had swapped the order not paying
attention and I didn't expect anything of it. Because there was no testcase failing
due to this.
Anyways this fixes the problem on the trunk by swapping the order in match.pd and
adding a comment of why the order is this way.

I will try to come up with a patch for GCC 9 and 10 series later on which fixes
the problem there too.

Note I didn't include the original testcase which requires the vectorizer and AVX-512f
as I can't figure out the right dg options to restrict it to avx-512f but I did come up
with a testcase which shows the problem and even more shows the problem with the 9/10
series as mentioned.

OK? Bootstrapped and tested on x86_64-linux-gnu.

	PR tree-optimization/102622

gcc/ChangeLog:

	* match.pd: Swap the order of a?pow2cst:0 and a?-1:0 transformations.
	Swap the order of a?0:pow2cst and a?0:-1 transformations.

gcc/testsuite/ChangeLog:

	* gcc.c-torture/execute/bitfld-10.c: New test.
---
 gcc/match.pd                                  | 26 ++++++++++++-------
 .../gcc.c-torture/execute/bitfld-10.c         | 24 +++++++++++++++++
 2 files changed, 41 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/gcc.c-torture/execute/bitfld-10.c

diff --git a/gcc/match.pd b/gcc/match.pd
index 9d7c1ac637f..c153e9a6e98 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -3949,15 +3949,16 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
     /* a ? 1 : 0 -> a if 0 and 1 are integral types. */
     (if (integer_onep (@1))
      (convert (convert:boolean_type_node @0)))
-    /* a ? -1 : 0 -> -a. */
-    (if (INTEGRAL_TYPE_P (type) && integer_all_onesp (@1))
-     (negate (convert (convert:boolean_type_node @0))))
     /* a ? powerof2cst : 0 -> a << (log2(powerof2cst)) */
     (if (INTEGRAL_TYPE_P (type) && integer_pow2p (@1))
      (with {
        tree shift = build_int_cst (integer_type_node, tree_log2 (@1));
       }
-      (lshift (convert (convert:boolean_type_node @0)) { shift; })))))
+      (lshift (convert (convert:boolean_type_node @0)) { shift; })))
+    /* a ? -1 : 0 -> -a.  No need to check the TYPE_PRECISION not being 1
+       here as the powerof2cst case above will handle that case correctly.  */
+    (if (INTEGRAL_TYPE_P (type) && integer_all_onesp (@1))
+     (negate (convert (convert:boolean_type_node @0))))))
   (if (integer_zerop (@1))
    (with {
       tree booltrue = constant_boolean_node (true, boolean_type_node);
@@ -3966,16 +3967,23 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
      /* a ? 0 : 1 -> !a. */
      (if (integer_onep (@2))
       (convert (bit_xor (convert:boolean_type_node @0) { booltrue; } )))
-     /* a ? -1 : 0 -> -(!a). */
-     (if (INTEGRAL_TYPE_P (type) && integer_all_onesp (@2))
-      (negate (convert (bit_xor (convert:boolean_type_node @0) { booltrue; } ))))
      /* a ? powerof2cst : 0 -> (!a) << (log2(powerof2cst)) */
-     (if (INTEGRAL_TYPE_P (type) &&  integer_pow2p (@2))
+     (if (INTEGRAL_TYPE_P (type) &&  integer_pow2p (@2)
+         && TYPE_PRECISION (type) != 1)
       (with {
 	tree shift = build_int_cst (integer_type_node, tree_log2 (@2));
        }
        (lshift (convert (bit_xor (convert:boolean_type_node @0) { booltrue; } ))
-        { shift; }))))))))
+        { shift; })))
+     /* a ? -1 : 0 -> -(!a).  No need to check the TYPE_PRECISION not being 1
+       here as the powerof2cst case above will handle that case correctly.  */
+     (if (INTEGRAL_TYPE_P (type) && integer_all_onesp (@2))
+      (negate (convert (bit_xor (convert:boolean_type_node @0) { booltrue; } ))))
+    )
+   )
+  )
+ )
+)
 #endif
 
 /* Simplification moved from fold_cond_expr_with_comparison.  It may also
diff --git a/gcc/testsuite/gcc.c-torture/execute/bitfld-10.c b/gcc/testsuite/gcc.c-torture/execute/bitfld-10.c
new file mode 100644
index 00000000000..bdbf5733ce7
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/bitfld-10.c
@@ -0,0 +1,24 @@
+/* PR tree-optimization/102622 */
+/* Wrong code introduced due to phi-opt
+   introducing undefined signed interger overflow
+   with one bit signed integer negation. */
+
+struct f{signed t:1;};
+int g(struct f *a, int t) __attribute__((noipa));
+int g(struct f *a, int t)
+{
+    if (t)
+      a->t = -1;
+    else
+      a->t = 0;
+    int t1 = a->t;
+    if (t1) return 1;
+    return t1;
+}
+
+int main(void)
+{
+    struct f a;
+    if (!g(&a, 1))  __builtin_abort();
+    return 0;
+}
-- 
2.17.1

Re: [PATCH] tree-optimization: [PR102622]: wrong code due to signed one bit integer and "a?-1:0"

[Richard Biener]

On October 10, 2021 7:42:19 AM GMT+02:00, apinski--- via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>From: Andrew Pinski <apinski@marvell.com>
>
>So it turns out this is kinda of a latent bug but not really latent.
>In GCC 9 and 10, phi-opt would transform a?-1:0 (even for signed 1-bit integer)
>to -(type)a but the type is an one bit integer which means the negation is
>undefined. GCC 11 fixed the problem by checking for a?pow2cst:0 transformation
>before a?-1:0 transformation.
>
>When I added the transformations to match.pd, I had swapped the order not paying
>attention and I didn't expect anything of it. Because there was no testcase failing
>due to this.
>Anyways this fixes the problem on the trunk by swapping the order in match.pd and
>adding a comment of why the order is this way.
>
>I will try to come up with a patch for GCC 9 and 10 series later on which fixes
>the problem there too.
>
>Note I didn't include the original testcase which requires the vectorizer and AVX-512f
>as I can't figure out the right dg options to restrict it to avx-512f but I did come up
>with a testcase which shows the problem and even more shows the problem with the 9/10
>series as mentioned.
>
>OK? Bootstrapped and tested on x86_64-linux-gnu.

OK. 

Richard. 


>	PR tree-optimization/102622
>
>gcc/ChangeLog:
>
>	* match.pd: Swap the order of a?pow2cst:0 and a?-1:0 transformations.
>	Swap the order of a?0:pow2cst and a?0:-1 transformations.
>
>gcc/testsuite/ChangeLog:
>
>	* gcc.c-torture/execute/bitfld-10.c: New test.
>---
> gcc/match.pd                                  | 26 ++++++++++++-------
> .../gcc.c-torture/execute/bitfld-10.c         | 24 +++++++++++++++++
> 2 files changed, 41 insertions(+), 9 deletions(-)
> create mode 100644 gcc/testsuite/gcc.c-torture/execute/bitfld-10.c
>
>diff --git a/gcc/match.pd b/gcc/match.pd
>index 9d7c1ac637f..c153e9a6e98 100644
>--- a/gcc/match.pd
>+++ b/gcc/match.pd
>@@ -3949,15 +3949,16 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>     /* a ? 1 : 0 -> a if 0 and 1 are integral types. */
>     (if (integer_onep (@1))
>      (convert (convert:boolean_type_node @0)))
>-    /* a ? -1 : 0 -> -a. */
>-    (if (INTEGRAL_TYPE_P (type) && integer_all_onesp (@1))
>-     (negate (convert (convert:boolean_type_node @0))))
>     /* a ? powerof2cst : 0 -> a << (log2(powerof2cst)) */
>     (if (INTEGRAL_TYPE_P (type) && integer_pow2p (@1))
>      (with {
>        tree shift = build_int_cst (integer_type_node, tree_log2 (@1));
>       }
>-      (lshift (convert (convert:boolean_type_node @0)) { shift; })))))
>+      (lshift (convert (convert:boolean_type_node @0)) { shift; })))
>+    /* a ? -1 : 0 -> -a.  No need to check the TYPE_PRECISION not being 1
>+       here as the powerof2cst case above will handle that case correctly.  */
>+    (if (INTEGRAL_TYPE_P (type) && integer_all_onesp (@1))
>+     (negate (convert (convert:boolean_type_node @0))))))
>   (if (integer_zerop (@1))
>    (with {
>       tree booltrue = constant_boolean_node (true, boolean_type_node);
>@@ -3966,16 +3967,23 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
>      /* a ? 0 : 1 -> !a. */
>      (if (integer_onep (@2))
>       (convert (bit_xor (convert:boolean_type_node @0) { booltrue; } )))
>-     /* a ? -1 : 0 -> -(!a). */
>-     (if (INTEGRAL_TYPE_P (type) && integer_all_onesp (@2))
>-      (negate (convert (bit_xor (convert:boolean_type_node @0) { booltrue; } ))))
>      /* a ? powerof2cst : 0 -> (!a) << (log2(powerof2cst)) */
>-     (if (INTEGRAL_TYPE_P (type) &&  integer_pow2p (@2))
>+     (if (INTEGRAL_TYPE_P (type) &&  integer_pow2p (@2)
>+         && TYPE_PRECISION (type) != 1)
>       (with {
> 	tree shift = build_int_cst (integer_type_node, tree_log2 (@2));
>        }
>        (lshift (convert (bit_xor (convert:boolean_type_node @0) { booltrue; } ))
>-        { shift; }))))))))
>+        { shift; })))
>+     /* a ? -1 : 0 -> -(!a).  No need to check the TYPE_PRECISION not being 1
>+       here as the powerof2cst case above will handle that case correctly.  */
>+     (if (INTEGRAL_TYPE_P (type) && integer_all_onesp (@2))
>+      (negate (convert (bit_xor (convert:boolean_type_node @0) { booltrue; } ))))
>+    )
>+   )
>+  )
>+ )
>+)
> #endif
> 
> /* Simplification moved from fold_cond_expr_with_comparison.  It may also
>diff --git a/gcc/testsuite/gcc.c-torture/execute/bitfld-10.c b/gcc/testsuite/gcc.c-torture/execute/bitfld-10.c
>new file mode 100644
>index 00000000000..bdbf5733ce7
>--- /dev/null
>+++ b/gcc/testsuite/gcc.c-torture/execute/bitfld-10.c
>@@ -0,0 +1,24 @@
>+/* PR tree-optimization/102622 */
>+/* Wrong code introduced due to phi-opt
>+   introducing undefined signed interger overflow
>+   with one bit signed integer negation. */
>+
>+struct f{signed t:1;};
>+int g(struct f *a, int t) __attribute__((noipa));
>+int g(struct f *a, int t)
>+{
>+    if (t)
>+      a->t = -1;
>+    else
>+      a->t = 0;
>+    int t1 = a->t;
>+    if (t1) return 1;
>+    return t1;
>+}
>+
>+int main(void)
>+{
>+    struct f a;
>+    if (!g(&a, 1))  __builtin_abort();
>+    return 0;
>+}

Re: [PATCH] tree-optimization: [PR102622]: wrong code due to signed one bit integer and "a?-1:0"

[Michael Matz]

Hello,

On Sat, 9 Oct 2021, apinski--- via Gcc-patches wrote:

> +      (lshift (convert (convert:boolean_type_node @0)) { shift; })))
> +    /* a ? -1 : 0 -> -a.  No need to check the TYPE_PRECISION not being 1
> +       here as the powerof2cst case above will handle that case correctly.  */

Well, but the QoI will improve quite a bit when you just do the check, 
instead of relying on order of patterns.  It's not slow or harmful to 
check and will make the order irrelevant, which, given the number of 
patterns we already have, is a good thing.  (It will also be smaller to 
check than to document why the check isn't needed :-) )


Ciao,
Michael.