diff mbox series

fold-const: Fix up multiple_of_p [PR112733]

Message ID ZWb2REgNh8xRoXg7@tucnak
State New
Headers show
Series fold-const: Fix up multiple_of_p [PR112733] | expand

Commit Message

Jakub Jelinek Nov. 29, 2023, 8:28 a.m. UTC 
Hi!

We ICE on the following testcase when wi::multiple_of_p is called on
widest_int 1 and -128 with UNSIGNED.  I still need to work on the
actual wide-int.cc issue, the latest patch attached to the PR regressed
bitint-{38,39}.c, so will need to debug that, but there is a clear bug
on the fold-const.cc side as well - widest_int is a signed representation
by definition, using UNSIGNED with it certainly doesn't match what was
intended, because -128 as the second operand effectively means unsigned
131072 bit 0xfffff............ffff80 integer, not the signed char -128
that appeared in the source.

In the INTEGER_CST case a few lines above this we already use
    case INTEGER_CST:
      if (TREE_CODE (bottom) != INTEGER_CST || integer_zerop (bottom))
        return false;
      return wi::multiple_of_p (wi::to_widest (top), wi::to_widest (bottom),
                                SIGNED);
so I think using SIGNED with widest_int is best there (compared to the
other choices in the PR).

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

2023-11-29  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/112733
	* fold-const.cc (multiple_of_p): Pass SIGNED rather than
	UNSIGNED for wi::multiple_of_p on widest_int arguments.

	* gcc.dg/pr112733.c: New test.


	Jakub

Comments

Richard Sandiford Nov. 29, 2023, 9:13 a.m. UTC | #1 
Jakub Jelinek <jakub@redhat.com> writes:
> Hi!
>
> We ICE on the following testcase when wi::multiple_of_p is called on
> widest_int 1 and -128 with UNSIGNED.  I still need to work on the
> actual wide-int.cc issue, the latest patch attached to the PR regressed
> bitint-{38,39}.c, so will need to debug that, but there is a clear bug
> on the fold-const.cc side as well - widest_int is a signed representation
> by definition, using UNSIGNED with it certainly doesn't match what was
> intended, because -128 as the second operand effectively means unsigned
> 131072 bit 0xfffff............ffff80 integer, not the signed char -128
> that appeared in the source.
>
> In the INTEGER_CST case a few lines above this we already use
>     case INTEGER_CST:
>       if (TREE_CODE (bottom) != INTEGER_CST || integer_zerop (bottom))
>         return false;
>       return wi::multiple_of_p (wi::to_widest (top), wi::to_widest (bottom),
>                                 SIGNED);
> so I think using SIGNED with widest_int is best there (compared to the
> other choices in the PR).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2023-11-29  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR middle-end/112733
> 	* fold-const.cc (multiple_of_p): Pass SIGNED rather than
> 	UNSIGNED for wi::multiple_of_p on widest_int arguments.
>
> 	* gcc.dg/pr112733.c: New test.

OK, thanks.

Richard

> --- gcc/fold-const.cc.jj	2023-11-28 08:46:28.345803059 +0100
> +++ gcc/fold-const.cc	2023-11-28 17:16:26.872291024 +0100
> @@ -14563,7 +14563,7 @@ multiple_of_p (tree type, const_tree top
>  	      && TREE_CODE (op2) == INTEGER_CST
>  	      && integer_pow2p (bottom)
>  	      && wi::multiple_of_p (wi::to_widest (op2),
> -				    wi::to_widest (bottom), UNSIGNED))
> +				    wi::to_widest (bottom), SIGNED))
>  	    return true;
>  
>  	  op1 = gimple_assign_rhs1 (stmt);
> --- gcc/testsuite/gcc.dg/pr112733.c.jj	2023-11-28 17:19:06.813048090 +0100
> +++ gcc/testsuite/gcc.dg/pr112733.c	2023-11-28 17:18:45.331349335 +0100
> @@ -0,0 +1,16 @@
> +/* PR middle-end/112733 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +signed char a, c;
> +short b;
> +
> +void
> +foo (void)
> +{
> +  signed char *e = &a;
> +  c = foo != 0;
> +  *e &= c;
> +  for (; b; --b)
> +    *e &= -128;
> +}
>
> 	Jakub
Richard Biener Nov. 29, 2023, 9:13 a.m. UTC | #2 
> Am 29.11.2023 um 09:29 schrieb Jakub Jelinek <jakub@redhat.com>:
> 
> Hi!
> 
> We ICE on the following testcase when wi::multiple_of_p is called on
> widest_int 1 and -128 with UNSIGNED.  I still need to work on the
> actual wide-int.cc issue, the latest patch attached to the PR regressed
> bitint-{38,39}.c, so will need to debug that, but there is a clear bug
> on the fold-const.cc side as well - widest_int is a signed representation
> by definition, using UNSIGNED with it certainly doesn't match what was
> intended, because -128 as the second operand effectively means unsigned
> 131072 bit 0xfffff............ffff80 integer, not the signed char -128
> that appeared in the source.
> 
> In the INTEGER_CST case a few lines above this we already use
>    case INTEGER_CST:
>      if (TREE_CODE (bottom) != INTEGER_CST || integer_zerop (bottom))
>        return false;
>      return wi::multiple_of_p (wi::to_widest (top), wi::to_widest (bottom),
>                                SIGNED);
> so I think using SIGNED with widest_int is best there (compared to the
> other choices in the PR).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok

Richard 

> 2023-11-29  Jakub Jelinek  <jakub@redhat.com>
> 
>    PR middle-end/112733
>    * fold-const.cc (multiple_of_p): Pass SIGNED rather than
>    UNSIGNED for wi::multiple_of_p on widest_int arguments.
> 
>    * gcc.dg/pr112733.c: New test.
> 
> --- gcc/fold-const.cc.jj    2023-11-28 08:46:28.345803059 +0100
> +++ gcc/fold-const.cc    2023-11-28 17:16:26.872291024 +0100
> @@ -14563,7 +14563,7 @@ multiple_of_p (tree type, const_tree top
>          && TREE_CODE (op2) == INTEGER_CST
>          && integer_pow2p (bottom)
>          && wi::multiple_of_p (wi::to_widest (op2),
> -                    wi::to_widest (bottom), UNSIGNED))
> +                    wi::to_widest (bottom), SIGNED))
>        return true;
> 
>      op1 = gimple_assign_rhs1 (stmt);
> --- gcc/testsuite/gcc.dg/pr112733.c.jj    2023-11-28 17:19:06.813048090 +0100
> +++ gcc/testsuite/gcc.dg/pr112733.c    2023-11-28 17:18:45.331349335 +0100
> @@ -0,0 +1,16 @@
> +/* PR middle-end/112733 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +signed char a, c;
> +short b;
> +
> +void
> +foo (void)
> +{
> +  signed char *e = &a;
> +  c = foo != 0;
> +  *e &= c;
> +  for (; b; --b)
> +    *e &= -128;
> +}
> 
>    Jakub
>
diff mbox series

Patch

--- gcc/fold-const.cc.jj	2023-11-28 08:46:28.345803059 +0100
+++ gcc/fold-const.cc	2023-11-28 17:16:26.872291024 +0100
@@ -14563,7 +14563,7 @@  multiple_of_p (tree type, const_tree top
 	      && TREE_CODE (op2) == INTEGER_CST
 	      && integer_pow2p (bottom)
 	      && wi::multiple_of_p (wi::to_widest (op2),
-				    wi::to_widest (bottom), UNSIGNED))
+				    wi::to_widest (bottom), SIGNED))
 	    return true;
 
 	  op1 = gimple_assign_rhs1 (stmt);
--- gcc/testsuite/gcc.dg/pr112733.c.jj	2023-11-28 17:19:06.813048090 +0100
+++ gcc/testsuite/gcc.dg/pr112733.c	2023-11-28 17:18:45.331349335 +0100
@@ -0,0 +1,16 @@ 
+/* PR middle-end/112733 */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+signed char a, c;
+short b;
+
+void
+foo (void)
+{
+  signed char *e = &a;
+  c = foo != 0;
+  *e &= c;
+  for (; b; --b)
+    *e &= -128;
+}