[gcc r12-2238] PR tree-optimization/101403: Incorrect folding of ((T)bswap(x))>>C

[gcc r12-2238] PR tree-optimization/101403: Incorrect folding of ((T)bswap(x))>>C

[Roger Sayle]

https://gcc.gnu.org/g:5f5fbb550af7d9d6cb56ae8f607fea0eccaa9295

commit r12-2238-g5f5fbb550af7d9d6cb56ae8f607fea0eccaa9295
Author: Roger Sayle <roger@nextmovesoftware.com>
Date:   Mon Jul 12 08:24:27 2021 +0100

    PR tree-optimization/101403: Incorrect folding of ((T)bswap(x))>>C
    
    My sincere apologies for the breakage.  My recent patch to fold
    bswapN(x)>>C where the constant C was large enough that the result
    only contains bits from the low byte, and can therefore avoid
    the byte swap contains a minor logic error.  The pattern contains
    a convert? allowing an extension to occur between the bswap and
    the shift.  The logic is correct if there's no extension, or the
    extension has the same sign as the shift, but I'd mistakenly
    convinced myself that these couldn't have different signedness.
    
    (T)bswap16(x)>>12 is (T)((unsigned char)x>>4) or (T)((signed char)x>>4).
    The bug is that for zero-extensions to signed type T, we need to use
    the unsigned char variant [the signedness of the byte shift is not
    (always) the same as the signedness of T and the original shift].
    
    Then because I'm now paranoid, I've also added a clause to handle
    the hypothetical (but in practice impossible) sign-extension to an
    unsigned type T, which can implemented as (T)(x<<8)>>12.
    
    2021-07-12  Roger Sayle  <roger@nextmovesoftware.com>
    
    gcc/ChangeLog
            PR tree-optimization/101403
            * match.pd ((T)bswap(X)>>C): Correctly handle cases where
            signedness of the shift is not the same as the signedness of
            the type extension.
    
    gcc/testsuite/ChangeLog
            PR tree-optimization/101403
            * gcc.dg/pr101403.c: New test case.

Diff:
---
 gcc/match.pd                    | 36 ++++++++++++++++++++++++------------
 gcc/testsuite/gcc.dg/pr101403.c | 17 +++++++++++++++++
 2 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/gcc/match.pd b/gcc/match.pd
index 30680d488ab..beb8d27535e 100644
--- a/gcc/match.pd
+++ b/gcc/match.pd
@@ -3659,19 +3659,31 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
      {
       unsigned HOST_WIDE_INT prec = TYPE_PRECISION (TREE_TYPE (@2));
       unsigned HOST_WIDE_INT bits = tree_to_uhwi (@1);
+      /* If the bswap was extended before the original shift, this
+	 byte (shift) has the sign of the extension, not the sign of
+	 the original shift.  */
+      tree st = TYPE_PRECISION (type) > prec ? TREE_TYPE (@2) : type;
      }
-     (if (bits + 8 == prec)
-      (if (TYPE_UNSIGNED (type))
-       (convert (convert:unsigned_char_type_node @0))
-       (convert (convert:signed_char_type_node @0)))
-      (if (bits < prec && bits + 8 > prec)
-       (with 
-	{
-	 tree nst = build_int_cst (integer_type_node, bits & 7);
-	 tree bt = TYPE_UNSIGNED (type) ? unsigned_char_type_node
-					: signed_char_type_node;
-	}
-	(convert (rshift:bt (convert:bt @0) {nst;}))))))))
+     /* Special case: logical right shift of sign-extended bswap.
+	(unsigned)(short)bswap16(x)>>12 is (unsigned)((short)x<<8)>>12. */
+     (if (TYPE_PRECISION (type) > prec
+	  && !TYPE_UNSIGNED (TREE_TYPE (@2))
+	  && TYPE_UNSIGNED (type)
+	  && bits < prec && bits + 8 >= prec)
+      (with { tree nst = build_int_cst (integer_type_node, prec - 8); }
+       (rshift (convert (lshift:st (convert:st @0) {nst;})) @1))
+      (if (bits + 8 == prec)
+       (if (TYPE_UNSIGNED (st))
+	(convert (convert:unsigned_char_type_node @0))
+	(convert (convert:signed_char_type_node @0)))
+       (if (bits < prec && bits + 8 > prec)
+	(with 
+	 {
+	  tree nst = build_int_cst (integer_type_node, bits & 7);
+	  tree bt = TYPE_UNSIGNED (st) ? unsigned_char_type_node
+				       : signed_char_type_node;
+	 }
+	 (convert (rshift:bt (convert:bt @0) {nst;})))))))))
  /* bswap(x) & C1 can sometimes be simplified to (x >> C2) & C1.  */
  (simplify
   (bit_and (convert? (bswap@2 @0)) INTEGER_CST@1)
diff --git a/gcc/testsuite/gcc.dg/pr101403.c b/gcc/testsuite/gcc.dg/pr101403.c
new file mode 100644
index 00000000000..ac5fa7944dc
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr101403.c
@@ -0,0 +1,17 @@
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+unsigned int foo (unsigned int a)
+{
+  unsigned int u;
+  unsigned short b = __builtin_bswap16 (a);
+  return b >> (u, 12);
+}
+
+int main (void)
+{
+  unsigned int x = foo (0x80);
+  if (x != 0x0008)
+    __builtin_abort ();
+  return 0;
+}
+