[PATCH] match.pd: Fix up recent __builtin_bswap16 simplifications [PR101642]

[PATCH] match.pd: Fix up recent __builtin_bswap16 simplifications [PR101642]

[Jakub Jelinek]

Hi!

The following testcase ICEs.  The problem is that for __builtin_bswap16
(and only that, others are fine) the argument of the builtin is promoted
to int while the patterns assume it is not and is the same as that of
the return type.
For the bswap simplifications before these new ones it just means we
fail to optimize stuff like __builtin_bswap16 (__builtin_bswap16 (x))
because there are casts in between, but the last one, equality comparison
of __builtin_bswap16 with integer constant results in ICE, because
we create comparison with incompatible types of the operands, and the
other might be fine because usually we bit and the operand before promoting,
but I think it is too dangerous to rely on it, one day we find out that
because it is operand to such a built in, we can throw away any changes
that affect the upper bits and all of sudden it would misbehave.

So, this patch introduces converts that shouldn't do anything for
bswap{32,64,128} and should fix these issues for bswap16.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2021-07-28  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/101642
	* match.pd (bswap16 (x) == bswap16 (y)): Cast both operands
	to type of bswap16 for comparison.
	(bswap16 (x) == cst): Cast bswap16 operand to type of cst.

	* gcc.c-torture/compile/pr101642.c: New test.

--- gcc/match.pd.jj	2021-07-27 09:47:44.000000000 +0200
+++ gcc/match.pd	2021-07-27 16:16:19.867757153 +0200
@@ -3641,11 +3641,13 @@ (define_operator_list COND_TERNARY
    (bitop @0 (bswap @1))))
  (for cmp (eq ne)
   (simplify
-   (cmp (bswap @0) (bswap @1))
-   (cmp @0 @1))
+   (cmp (bswap@2 @0) (bswap @1))
+   (with { tree ctype = TREE_TYPE (@2); }
+    (cmp (convert:ctype @0) (convert:ctype @1))))
   (simplify
    (cmp (bswap @0) INTEGER_CST@1)
-   (cmp @0 (bswap @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.c-torture/compile/pr101642.c.jj	2021-07-27 16:34:44.910039793 +0200
+++ gcc/testsuite/gcc.c-torture/compile/pr101642.c	2021-07-27 16:32:24.661907592 +0200
@@ -0,0 +1,17 @@
+/* PR middle-end/101642 */
+
+int x;
+
+unsigned short
+foo (void)
+{
+  return __builtin_bswap16 (x) ? : 0;
+}
+
+int
+bar (int x, int y)
+{
+  unsigned short a = __builtin_bswap16 ((unsigned short) x);
+  unsigned short b = __builtin_bswap16 ((unsigned short) y);
+  return a == b;
+}

	Jakub

Re: [PATCH] match.pd: Fix up recent __builtin_bswap16 simplifications [PR101642]

[Richard Biener]

On Wed, 28 Jul 2021, Jakub Jelinek wrote:

> Hi!
> 
> The following testcase ICEs.  The problem is that for __builtin_bswap16
> (and only that, others are fine) the argument of the builtin is promoted
> to int while the patterns assume it is not and is the same as that of
> the return type.
> For the bswap simplifications before these new ones it just means we
> fail to optimize stuff like __builtin_bswap16 (__builtin_bswap16 (x))
> because there are casts in between, but the last one, equality comparison
> of __builtin_bswap16 with integer constant results in ICE, because
> we create comparison with incompatible types of the operands, and the
> other might be fine because usually we bit and the operand before promoting,
> but I think it is too dangerous to rely on it, one day we find out that
> because it is operand to such a built in, we can throw away any changes
> that affect the upper bits and all of sudden it would misbehave.
> 
> So, this patch introduces converts that shouldn't do anything for
> bswap{32,64,128} and should fix these issues for bswap16.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Richard.

> 2021-07-28  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/101642
> 	* match.pd (bswap16 (x) == bswap16 (y)): Cast both operands
> 	to type of bswap16 for comparison.
> 	(bswap16 (x) == cst): Cast bswap16 operand to type of cst.
> 
> 	* gcc.c-torture/compile/pr101642.c: New test.
> 
> --- gcc/match.pd.jj	2021-07-27 09:47:44.000000000 +0200
> +++ gcc/match.pd	2021-07-27 16:16:19.867757153 +0200
> @@ -3641,11 +3641,13 @@ (define_operator_list COND_TERNARY
>     (bitop @0 (bswap @1))))
>   (for cmp (eq ne)
>    (simplify
> -   (cmp (bswap @0) (bswap @1))
> -   (cmp @0 @1))
> +   (cmp (bswap@2 @0) (bswap @1))
> +   (with { tree ctype = TREE_TYPE (@2); }
> +    (cmp (convert:ctype @0) (convert:ctype @1))))
>    (simplify
>     (cmp (bswap @0) INTEGER_CST@1)
> -   (cmp @0 (bswap @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.c-torture/compile/pr101642.c.jj	2021-07-27 16:34:44.910039793 +0200
> +++ gcc/testsuite/gcc.c-torture/compile/pr101642.c	2021-07-27 16:32:24.661907592 +0200
> @@ -0,0 +1,17 @@
> +/* PR middle-end/101642 */
> +
> +int x;
> +
> +unsigned short
> +foo (void)
> +{
> +  return __builtin_bswap16 (x) ? : 0;
> +}
> +
> +int
> +bar (int x, int y)
> +{
> +  unsigned short a = __builtin_bswap16 ((unsigned short) x);
> +  unsigned short b = __builtin_bswap16 ((unsigned short) y);
> +  return a == b;
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)