[PATCH] expr: Fix REDUCE_BIT_FIELD in multiplication expansion [PR114054]

[PATCH] expr: Fix REDUCE_BIT_FIELD in multiplication expansion [PR114054]

[Jakub Jelinek]

Hi!

The following testcase ICEs, because the REDUCE_BIT_FIELD macro uses
the target variable implicitly:
#define REDUCE_BIT_FIELD(expr)  (reduce_bit_field                         \
                                 ? reduce_to_bit_field_precision ((expr), \
                                                                  target, \
                                                                  type)   \
                                 : (expr))
and so when the code below reuses the target variable, documented to be
   The value may be stored in TARGET if TARGET is nonzero.
   TARGET is just a suggestion; callers must assume that
   the rtx returned may not be the same as TARGET.
for something unrelated (the value that should be returned), this misbehaves
(in the testcase target is set to a CONST_INT, which has VOIDmode and
reduce_to_bit_field_precision assert checking doesn't like that).
Needed to say that
   If TARGET is CONST0_RTX, it means that the value will be ignored.
but in expand_expr_real_2 does at the start:
  ignore = (target == const0_rtx
            || ((CONVERT_EXPR_CODE_P (code)
                 || code == COND_EXPR || code == VIEW_CONVERT_EXPR)
                && TREE_CODE (type) == VOID_TYPE));

  /* We should be called only if we need the result.  */
  gcc_assert (!ignore);
- so such target is mainly meant for calls and the like in other routines.
Certainly doesn't expect that target changes from not being ignored
initially to ignore later on and other CONST_INT results as well as anything
which is not an object into which anything can be stored.

So, the following patch fixes that by using a more appripriate temporary
for the result, which other code is using.

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

2024-02-23  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/114054
	* expr.cc (expand_expr_real_2) <case MULT_EXPR>: Use
	temp variable instead of target parameter for result.

	* gcc.dg/bitint-92.c: New test.

--- gcc/expr.cc.jj	2024-02-14 14:26:19.709811397 +0100
+++ gcc/expr.cc	2024-02-22 18:39:37.852431789 +0100
@@ -10259,12 +10259,12 @@ expand_expr_real_2 (sepops ops, rtx targ
 					  &algorithm, &variant, cost)
 		  : cost < mul_cost (speed, mode))
 		{
-		  target = bit0_p ? expand_and (mode, negate_rtx (mode, op0),
-						op1, target)
-				  : expand_and (mode, op0,
-						negate_rtx (mode, op1),
-						target);
-		  return REDUCE_BIT_FIELD (target);
+		  temp = bit0_p ? expand_and (mode, negate_rtx (mode, op0),
+					      op1, target)
+				: expand_and (mode, op0,
+					      negate_rtx (mode, op1),
+					      target);
+		  return REDUCE_BIT_FIELD (temp);
 		}
 	    }
 	}
--- gcc/testsuite/gcc.dg/bitint-92.c.jj	2024-02-22 18:43:56.433910671 +0100
+++ gcc/testsuite/gcc.dg/bitint-92.c	2024-02-22 18:43:29.464277919 +0100
@@ -0,0 +1,17 @@
+/* PR rtl-optimization/114054 */
+/* { dg-do compile { target bitint } } */
+/* { dg-options "-Og -fwhole-program -fno-tree-ccp -fprofile-use -fno-tree-copy-prop -w" } */
+
+int x;
+
+void
+foo (int i, unsigned u)
+{
+  x = __builtin_mul_overflow_p ((unsigned _BitInt(1)) u, i, (_BitInt(33)) 0);
+}
+
+int
+main ()
+{
+  foo (11, 0);
+}

	Jakub

Re: [PATCH] expr: Fix REDUCE_BIT_FIELD in multiplication expansion [PR114054]

[Richard Biener]

On Fri, 23 Feb 2024, Jakub Jelinek wrote:

> Hi!
> 
> The following testcase ICEs, because the REDUCE_BIT_FIELD macro uses
> the target variable implicitly:
> #define REDUCE_BIT_FIELD(expr)  (reduce_bit_field                         \
>                                  ? reduce_to_bit_field_precision ((expr), \
>                                                                   target, \
>                                                                   type)   \
>                                  : (expr))
> and so when the code below reuses the target variable, documented to be
>    The value may be stored in TARGET if TARGET is nonzero.
>    TARGET is just a suggestion; callers must assume that
>    the rtx returned may not be the same as TARGET.
> for something unrelated (the value that should be returned), this misbehaves
> (in the testcase target is set to a CONST_INT, which has VOIDmode and
> reduce_to_bit_field_precision assert checking doesn't like that).
> Needed to say that
>    If TARGET is CONST0_RTX, it means that the value will be ignored.
> but in expand_expr_real_2 does at the start:
>   ignore = (target == const0_rtx
>             || ((CONVERT_EXPR_CODE_P (code)
>                  || code == COND_EXPR || code == VIEW_CONVERT_EXPR)
>                 && TREE_CODE (type) == VOID_TYPE));
> 
>   /* We should be called only if we need the result.  */
>   gcc_assert (!ignore);
> - so such target is mainly meant for calls and the like in other routines.
> Certainly doesn't expect that target changes from not being ignored
> initially to ignore later on and other CONST_INT results as well as anything
> which is not an object into which anything can be stored.
> 
> So, the following patch fixes that by using a more appripriate temporary
> for the result, which other code is using.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Thanks,
Richard.

> 2024-02-23  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR rtl-optimization/114054
> 	* expr.cc (expand_expr_real_2) <case MULT_EXPR>: Use
> 	temp variable instead of target parameter for result.
> 
> 	* gcc.dg/bitint-92.c: New test.
> 
> --- gcc/expr.cc.jj	2024-02-14 14:26:19.709811397 +0100
> +++ gcc/expr.cc	2024-02-22 18:39:37.852431789 +0100
> @@ -10259,12 +10259,12 @@ expand_expr_real_2 (sepops ops, rtx targ
>  					  &algorithm, &variant, cost)
>  		  : cost < mul_cost (speed, mode))
>  		{
> -		  target = bit0_p ? expand_and (mode, negate_rtx (mode, op0),
> -						op1, target)
> -				  : expand_and (mode, op0,
> -						negate_rtx (mode, op1),
> -						target);
> -		  return REDUCE_BIT_FIELD (target);
> +		  temp = bit0_p ? expand_and (mode, negate_rtx (mode, op0),
> +					      op1, target)
> +				: expand_and (mode, op0,
> +					      negate_rtx (mode, op1),
> +					      target);
> +		  return REDUCE_BIT_FIELD (temp);
>  		}
>  	    }
>  	}
> --- gcc/testsuite/gcc.dg/bitint-92.c.jj	2024-02-22 18:43:56.433910671 +0100
> +++ gcc/testsuite/gcc.dg/bitint-92.c	2024-02-22 18:43:29.464277919 +0100
> @@ -0,0 +1,17 @@
> +/* PR rtl-optimization/114054 */
> +/* { dg-do compile { target bitint } } */
> +/* { dg-options "-Og -fwhole-program -fno-tree-ccp -fprofile-use -fno-tree-copy-prop -w" } */
> +
> +int x;
> +
> +void
> +foo (int i, unsigned u)
> +{
> +  x = __builtin_mul_overflow_p ((unsigned _BitInt(1)) u, i, (_BitInt(33)) 0);
> +}
> +
> +int
> +main ()
> +{
> +  foo (11, 0);
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)