[PATCH] expr: Fix REDUCE_BIT_FIELD for constants [PR95694]

[PATCH] expr: Fix REDUCE_BIT_FIELD for constants [PR95694]

[Richard Sandiford]

[Sorry, been sitting on this patch for a while and just realised
 I never sent it.]

This is yet another PR caused by constant integer rtxes not storing
a mode.  We were calling REDUCE_BIT_FIELD on a constant integer that
didn't fit in poly_int64, and then tripped the as_a<scalar_int_mode>
assert on VOIDmode.

AFAICT REDUCE_BIT_FIELD is always passed rtxes that have TYPE_MODE
(rather than some other mode) and it just fills in the redundant
sign bits of that TYPE_MODE value.  So it should be safe to get
the mode from the type instead of the rtx.  The patch does that
and asserts that the modes agree, where information is available.

That on its own is enough to fix the bug, but we might as well
extend the folding case to all constant integers, not just those
that fit poly_int64.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to trunk
and release branches?

Richard


gcc/
	PR middle-end/95694
	* expr.c (expand_expr_real_2): Get the mode from the type rather
	than the rtx, and assert that it is consistent with the mode of
	the rtx (where known).  Optimize all constant integers, not just
	those that can be represented in poly_int64.

gcc/testsuite/
	PR middle-end/95694
	* gcc.dg/pr95694.c: New test.
---
 gcc/expr.c                     | 15 ++++++++-------
 gcc/testsuite/gcc.dg/pr95694.c | 23 +++++++++++++++++++++++
 2 files changed, 31 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr95694.c

diff --git a/gcc/expr.c b/gcc/expr.c
index 3c68b0d754c..715edae819a 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -11525,26 +11525,27 @@ expand_expr_real_1 (tree exp, rtx target, machine_mode tmode,
 static rtx
 reduce_to_bit_field_precision (rtx exp, rtx target, tree type)
 {
+  scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
   HOST_WIDE_INT prec = TYPE_PRECISION (type);
-  if (target && GET_MODE (target) != GET_MODE (exp))
+  gcc_assert (GET_MODE (exp) == VOIDmode || GET_MODE (exp) == mode);
+  if (target && GET_MODE (target) != mode)
     target = 0;
-  /* For constant values, reduce using build_int_cst_type. */
-  poly_int64 const_exp;
-  if (poly_int_rtx_p (exp, &const_exp))
+
+  /* For constant values, reduce using wide_int_to_tree. */
+  if (poly_int_rtx_p (exp))
     {
-      tree t = build_int_cst_type (type, const_exp);
+      auto value = wi::to_poly_wide (exp, mode);
+      tree t = wide_int_to_tree (type, value);
       return expand_expr (t, target, VOIDmode, EXPAND_NORMAL);
     }
   else if (TYPE_UNSIGNED (type))
     {
-      scalar_int_mode mode = as_a <scalar_int_mode> (GET_MODE (exp));
       rtx mask = immed_wide_int_const
 	(wi::mask (prec, false, GET_MODE_PRECISION (mode)), mode);
       return expand_and (mode, exp, mask, target);
     }
   else
     {
-      scalar_int_mode mode = as_a <scalar_int_mode> (GET_MODE (exp));
       int count = GET_MODE_PRECISION (mode) - prec;
       exp = expand_shift (LSHIFT_EXPR, mode, exp, count, target, 0);
       return expand_shift (RSHIFT_EXPR, mode, exp, count, target, 0);
diff --git a/gcc/testsuite/gcc.dg/pr95694.c b/gcc/testsuite/gcc.dg/pr95694.c
new file mode 100644
index 00000000000..6f5e1900a02
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr95694.c
@@ -0,0 +1,23 @@
+/* PR tree-optimization/68835 */
+/* { dg-do run { target int128 } } */
+/* { dg-options "-fno-tree-forwprop -fno-tree-ccp -O1 -fno-tree-dominator-opts -fno-tree-fre" } */
+
+__attribute__((noinline, noclone)) unsigned __int128
+foo (void)
+{
+  unsigned __int128 x = (unsigned __int128) 0xffffffffffffffffULL;
+  struct { unsigned __int128 a : 65; } w;
+  w.a = x;
+  w.a += x;
+  return w.a;
+}
+
+int
+main ()
+{
+  unsigned __int128 x = foo ();
+  if ((unsigned long long) x != 0xfffffffffffffffeULL
+      || (unsigned long long) (x >> 64) != 1)
+    __builtin_abort ();
+  return 0;
+}

Re: [PATCH] expr: Fix REDUCE_BIT_FIELD for constants [PR95694]

[Richard Biener]

On Tue, Jul 7, 2020 at 9:20 PM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> [Sorry, been sitting on this patch for a while and just realised
>  I never sent it.]
>
> This is yet another PR caused by constant integer rtxes not storing
> a mode.  We were calling REDUCE_BIT_FIELD on a constant integer that
> didn't fit in poly_int64, and then tripped the as_a<scalar_int_mode>
> assert on VOIDmode.
>
> AFAICT REDUCE_BIT_FIELD is always passed rtxes that have TYPE_MODE
> (rather than some other mode) and it just fills in the redundant
> sign bits of that TYPE_MODE value.  So it should be safe to get
> the mode from the type instead of the rtx.  The patch does that
> and asserts that the modes agree, where information is available.
>
> That on its own is enough to fix the bug, but we might as well
> extend the folding case to all constant integers, not just those
> that fit poly_int64.
>
> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to trunk
> and release branches?

OK.

Thanks,
Richard.

> Richard
>
>
> gcc/
>         PR middle-end/95694
>         * expr.c (expand_expr_real_2): Get the mode from the type rather
>         than the rtx, and assert that it is consistent with the mode of
>         the rtx (where known).  Optimize all constant integers, not just
>         those that can be represented in poly_int64.
>
> gcc/testsuite/
>         PR middle-end/95694
>         * gcc.dg/pr95694.c: New test.
> ---
>  gcc/expr.c                     | 15 ++++++++-------
>  gcc/testsuite/gcc.dg/pr95694.c | 23 +++++++++++++++++++++++
>  2 files changed, 31 insertions(+), 7 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/pr95694.c
>
> diff --git a/gcc/expr.c b/gcc/expr.c
> index 3c68b0d754c..715edae819a 100644
> --- a/gcc/expr.c
> +++ b/gcc/expr.c
> @@ -11525,26 +11525,27 @@ expand_expr_real_1 (tree exp, rtx target, machine_mode tmode,
>  static rtx
>  reduce_to_bit_field_precision (rtx exp, rtx target, tree type)
>  {
> +  scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
>    HOST_WIDE_INT prec = TYPE_PRECISION (type);
> -  if (target && GET_MODE (target) != GET_MODE (exp))
> +  gcc_assert (GET_MODE (exp) == VOIDmode || GET_MODE (exp) == mode);
> +  if (target && GET_MODE (target) != mode)
>      target = 0;
> -  /* For constant values, reduce using build_int_cst_type. */
> -  poly_int64 const_exp;
> -  if (poly_int_rtx_p (exp, &const_exp))
> +
> +  /* For constant values, reduce using wide_int_to_tree. */
> +  if (poly_int_rtx_p (exp))
>      {
> -      tree t = build_int_cst_type (type, const_exp);
> +      auto value = wi::to_poly_wide (exp, mode);
> +      tree t = wide_int_to_tree (type, value);
>        return expand_expr (t, target, VOIDmode, EXPAND_NORMAL);
>      }
>    else if (TYPE_UNSIGNED (type))
>      {
> -      scalar_int_mode mode = as_a <scalar_int_mode> (GET_MODE (exp));
>        rtx mask = immed_wide_int_const
>         (wi::mask (prec, false, GET_MODE_PRECISION (mode)), mode);
>        return expand_and (mode, exp, mask, target);
>      }
>    else
>      {
> -      scalar_int_mode mode = as_a <scalar_int_mode> (GET_MODE (exp));
>        int count = GET_MODE_PRECISION (mode) - prec;
>        exp = expand_shift (LSHIFT_EXPR, mode, exp, count, target, 0);
>        return expand_shift (RSHIFT_EXPR, mode, exp, count, target, 0);
> diff --git a/gcc/testsuite/gcc.dg/pr95694.c b/gcc/testsuite/gcc.dg/pr95694.c
> new file mode 100644
> index 00000000000..6f5e1900a02
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr95694.c
> @@ -0,0 +1,23 @@
> +/* PR tree-optimization/68835 */
> +/* { dg-do run { target int128 } } */
> +/* { dg-options "-fno-tree-forwprop -fno-tree-ccp -O1 -fno-tree-dominator-opts -fno-tree-fre" } */
> +
> +__attribute__((noinline, noclone)) unsigned __int128
> +foo (void)
> +{
> +  unsigned __int128 x = (unsigned __int128) 0xffffffffffffffffULL;
> +  struct { unsigned __int128 a : 65; } w;
> +  w.a = x;
> +  w.a += x;
> +  return w.a;
> +}
> +
> +int
> +main ()
> +{
> +  unsigned __int128 x = foo ();
> +  if ((unsigned long long) x != 0xfffffffffffffffeULL
> +      || (unsigned long long) (x >> 64) != 1)
> +    __builtin_abort ();
> +  return 0;
> +}

[GCC 10 PATCH] expr: Fix REDUCE_BIT_FIELD for constants [PR95694]

[Richard Sandiford]

This is a gcc-10 version of:

Richard Sandiford <richard.sandiford@arm.com> writes:
> [Sorry, been sitting on this patch for a while and just realised
>  I never sent it.]
>
> This is yet another PR caused by constant integer rtxes not storing
> a mode.  We were calling REDUCE_BIT_FIELD on a constant integer that
> didn't fit in poly_int64, and then tripped the as_a<scalar_int_mode>
> assert on VOIDmode.
>
> AFAICT REDUCE_BIT_FIELD is always passed rtxes that have TYPE_MODE
> (rather than some other mode) and it just fills in the redundant
> sign bits of that TYPE_MODE value.  So it should be safe to get
> the mode from the type instead of the rtx.  The patch does that
> and asserts that the modes agree, where information is available.
>
> That on its own is enough to fix the bug, but we might as well
> extend the folding case to all constant integers, not just those
> that fit poly_int64.
>
> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to trunk
> and release branches?

The mainline patch ended up causing PR96151, so this patch folds
in the fix for that.  It also avoids some C++11-isms.

Tested on aarch64-linux-gnu, aarch64_be-elf, arm-linux-gnueabihf,
armeb-elf and x86_64-linux-gnu.  OK for GCC 10?

(I didn't include PR96151 in the changelog since it's no longer
a live bug.)

Richard


gcc/
	PR middle-end/95694
	* expr.c (expand_expr_real_2): Get the mode from the type rather
	than the rtx, and assert that it is consistent with the mode of
	the rtx (where known).  Optimize all constant integers, not just
	those that can be represented in poly_int64.

gcc/testsuite/
	PR middle-end/95694
	* gcc.dg/pr95694.c: New test.

(cherry picked from commit 760df6d296b8fc59796f42dca5eb14012fbfa28b)
---
 gcc/expr.c                     | 19 ++++++++++---------
 gcc/testsuite/gcc.dg/pr95694.c | 23 +++++++++++++++++++++++
 2 files changed, 33 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr95694.c

diff --git a/gcc/expr.c b/gcc/expr.c
index 6dfee6627a3..991b26f3341 100644
--- a/gcc/expr.c
+++ b/gcc/expr.c
@@ -8560,7 +8560,9 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode,
   reduce_bit_field = (INTEGRAL_TYPE_P (type)
 		      && !type_has_mode_precision_p (type));
 
-  if (reduce_bit_field && modifier == EXPAND_STACK_PARM)
+  if (reduce_bit_field
+      && (modifier == EXPAND_STACK_PARM
+	  || (target && GET_MODE (target) != mode)))
     target = 0;
 
   /* Use subtarget as the target for operand 0 of a binary operation.  */
@@ -11434,26 +11436,25 @@ expand_expr_real_1 (tree exp, rtx target, machine_mode tmode,
 static rtx
 reduce_to_bit_field_precision (rtx exp, rtx target, tree type)
 {
+  scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
   HOST_WIDE_INT prec = TYPE_PRECISION (type);
-  if (target && GET_MODE (target) != GET_MODE (exp))
-    target = 0;
-  /* For constant values, reduce using build_int_cst_type. */
-  poly_int64 const_exp;
-  if (poly_int_rtx_p (exp, &const_exp))
+  gcc_assert ((GET_MODE (exp) == VOIDmode || GET_MODE (exp) == mode)
+	      && (!target || GET_MODE (target) == mode));
+
+  /* For constant values, reduce using wide_int_to_tree. */
+  if (poly_int_rtx_p (exp))
     {
-      tree t = build_int_cst_type (type, const_exp);
+      tree t = wide_int_to_tree (type, wi::to_poly_wide (exp, mode));
       return expand_expr (t, target, VOIDmode, EXPAND_NORMAL);
     }
   else if (TYPE_UNSIGNED (type))
     {
-      scalar_int_mode mode = as_a <scalar_int_mode> (GET_MODE (exp));
       rtx mask = immed_wide_int_const
 	(wi::mask (prec, false, GET_MODE_PRECISION (mode)), mode);
       return expand_and (mode, exp, mask, target);
     }
   else
     {
-      scalar_int_mode mode = as_a <scalar_int_mode> (GET_MODE (exp));
       int count = GET_MODE_PRECISION (mode) - prec;
       exp = expand_shift (LSHIFT_EXPR, mode, exp, count, target, 0);
       return expand_shift (RSHIFT_EXPR, mode, exp, count, target, 0);
diff --git a/gcc/testsuite/gcc.dg/pr95694.c b/gcc/testsuite/gcc.dg/pr95694.c
new file mode 100644
index 00000000000..6f5e1900a02
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr95694.c
@@ -0,0 +1,23 @@
+/* PR tree-optimization/68835 */
+/* { dg-do run { target int128 } } */
+/* { dg-options "-fno-tree-forwprop -fno-tree-ccp -O1 -fno-tree-dominator-opts -fno-tree-fre" } */
+
+__attribute__((noinline, noclone)) unsigned __int128
+foo (void)
+{
+  unsigned __int128 x = (unsigned __int128) 0xffffffffffffffffULL;
+  struct { unsigned __int128 a : 65; } w;
+  w.a = x;
+  w.a += x;
+  return w.a;
+}
+
+int
+main ()
+{
+  unsigned __int128 x = foo ();
+  if ((unsigned long long) x != 0xfffffffffffffffeULL
+      || (unsigned long long) (x >> 64) != 1)
+    __builtin_abort ();
+  return 0;
+}

Re: [GCC 10 PATCH] expr: Fix REDUCE_BIT_FIELD for constants [PR95694]

[Richard Biener]

On Wed, Dec 2, 2020 at 5:31 PM Richard Sandiford via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> This is a gcc-10 version of:
>
> Richard Sandiford <richard.sandiford@arm.com> writes:
> > [Sorry, been sitting on this patch for a while and just realised
> >  I never sent it.]
> >
> > This is yet another PR caused by constant integer rtxes not storing
> > a mode.  We were calling REDUCE_BIT_FIELD on a constant integer that
> > didn't fit in poly_int64, and then tripped the as_a<scalar_int_mode>
> > assert on VOIDmode.
> >
> > AFAICT REDUCE_BIT_FIELD is always passed rtxes that have TYPE_MODE
> > (rather than some other mode) and it just fills in the redundant
> > sign bits of that TYPE_MODE value.  So it should be safe to get
> > the mode from the type instead of the rtx.  The patch does that
> > and asserts that the modes agree, where information is available.
> >
> > That on its own is enough to fix the bug, but we might as well
> > extend the folding case to all constant integers, not just those
> > that fit poly_int64.
> >
> > Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to trunk
> > and release branches?
>
> The mainline patch ended up causing PR96151, so this patch folds
> in the fix for that.  It also avoids some C++11-isms.
>
> Tested on aarch64-linux-gnu, aarch64_be-elf, arm-linux-gnueabihf,
> armeb-elf and x86_64-linux-gnu.  OK for GCC 10?

OK.

> (I didn't include PR96151 in the changelog since it's no longer
> a live bug.)
>
> Richard
>
>
> gcc/
>         PR middle-end/95694
>         * expr.c (expand_expr_real_2): Get the mode from the type rather
>         than the rtx, and assert that it is consistent with the mode of
>         the rtx (where known).  Optimize all constant integers, not just
>         those that can be represented in poly_int64.
>
> gcc/testsuite/
>         PR middle-end/95694
>         * gcc.dg/pr95694.c: New test.
>
> (cherry picked from commit 760df6d296b8fc59796f42dca5eb14012fbfa28b)
> ---
>  gcc/expr.c                     | 19 ++++++++++---------
>  gcc/testsuite/gcc.dg/pr95694.c | 23 +++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 9 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/pr95694.c
>
> diff --git a/gcc/expr.c b/gcc/expr.c
> index 6dfee6627a3..991b26f3341 100644
> --- a/gcc/expr.c
> +++ b/gcc/expr.c
> @@ -8560,7 +8560,9 @@ expand_expr_real_2 (sepops ops, rtx target, machine_mode tmode,
>    reduce_bit_field = (INTEGRAL_TYPE_P (type)
>                       && !type_has_mode_precision_p (type));
>
> -  if (reduce_bit_field && modifier == EXPAND_STACK_PARM)
> +  if (reduce_bit_field
> +      && (modifier == EXPAND_STACK_PARM
> +         || (target && GET_MODE (target) != mode)))
>      target = 0;
>
>    /* Use subtarget as the target for operand 0 of a binary operation.  */
> @@ -11434,26 +11436,25 @@ expand_expr_real_1 (tree exp, rtx target, machine_mode tmode,
>  static rtx
>  reduce_to_bit_field_precision (rtx exp, rtx target, tree type)
>  {
> +  scalar_int_mode mode = SCALAR_INT_TYPE_MODE (type);
>    HOST_WIDE_INT prec = TYPE_PRECISION (type);
> -  if (target && GET_MODE (target) != GET_MODE (exp))
> -    target = 0;
> -  /* For constant values, reduce using build_int_cst_type. */
> -  poly_int64 const_exp;
> -  if (poly_int_rtx_p (exp, &const_exp))
> +  gcc_assert ((GET_MODE (exp) == VOIDmode || GET_MODE (exp) == mode)
> +             && (!target || GET_MODE (target) == mode));
> +
> +  /* For constant values, reduce using wide_int_to_tree. */
> +  if (poly_int_rtx_p (exp))
>      {
> -      tree t = build_int_cst_type (type, const_exp);
> +      tree t = wide_int_to_tree (type, wi::to_poly_wide (exp, mode));
>        return expand_expr (t, target, VOIDmode, EXPAND_NORMAL);
>      }
>    else if (TYPE_UNSIGNED (type))
>      {
> -      scalar_int_mode mode = as_a <scalar_int_mode> (GET_MODE (exp));
>        rtx mask = immed_wide_int_const
>         (wi::mask (prec, false, GET_MODE_PRECISION (mode)), mode);
>        return expand_and (mode, exp, mask, target);
>      }
>    else
>      {
> -      scalar_int_mode mode = as_a <scalar_int_mode> (GET_MODE (exp));
>        int count = GET_MODE_PRECISION (mode) - prec;
>        exp = expand_shift (LSHIFT_EXPR, mode, exp, count, target, 0);
>        return expand_shift (RSHIFT_EXPR, mode, exp, count, target, 0);
> diff --git a/gcc/testsuite/gcc.dg/pr95694.c b/gcc/testsuite/gcc.dg/pr95694.c
> new file mode 100644
> index 00000000000..6f5e1900a02
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr95694.c
> @@ -0,0 +1,23 @@
> +/* PR tree-optimization/68835 */
> +/* { dg-do run { target int128 } } */
> +/* { dg-options "-fno-tree-forwprop -fno-tree-ccp -O1 -fno-tree-dominator-opts -fno-tree-fre" } */
> +
> +__attribute__((noinline, noclone)) unsigned __int128
> +foo (void)
> +{
> +  unsigned __int128 x = (unsigned __int128) 0xffffffffffffffffULL;
> +  struct { unsigned __int128 a : 65; } w;
> +  w.a = x;
> +  w.a += x;
> +  return w.a;
> +}
> +
> +int
> +main ()
> +{
> +  unsigned __int128 x = foo ();
> +  if ((unsigned long long) x != 0xfffffffffffffffeULL
> +      || (unsigned long long) (x >> 64) != 1)
> +    __builtin_abort ();
> +  return 0;
> +}