[PATCH] Modify simplify_truncation to handle extended CONST_INT.

[PATCH] Modify simplify_truncation to handle extended CONST_INT.

[Jim Wilson]

This addresses PR 91860 which has four testcases triggering internal errors.
The problem here is that in combine when handling debug insns, we are trying
to substitute
(sign_extend:DI (const_int 8160 [0x1fe0]))
as the value for
(reg:DI 78 [ _9 ])
in the debug insn
(debug_insn 29 28 30 2 (var_location:QI d (subreg:QI (reg:DI 78 [ _9 ]) 0)) "tmp4.c":11:5 -1
     (nil))

The place where this starts to go wrong is in simplify_truncation, where it
tries to compare the size of the original mode VOIDmode with the subreg mode
QI and decides that we need to sign extend the constant to convert it from
VOIDmode to QImode.  We actually need a truncation not a extension here.  Also
note that the GET_MODE_UNIT_PRECISION (VOIDmode) isn't useful.  We can fix
this by changing the mode to MAX_MODE_INT, as the CONST_INT should already be
valid for the largest supported integer mode.  There are already a number of
other places in simplify-rtx.c that do the same thing.

This was tested with rv32/newlib and rv64/linux cross builds and make checks.
There were no regressions.  The new tests all fail for rv64 without the patch,
and work with the patch.

OK?

Jim

	gcc/
	PR rtl-optimization/91860
	* simplify-rtx.c (simplify_truncation): If origmode is VOIDmode, set
	it to MAX_MODE_INT.

	gcc/testsuite/
	PR rtl-optimization/91860
	* gcc.dg/pr91860-1.c: New testcase.
	* gcc.dg/pr91860-2.c: New testcase.
	* gcc.dg/pr91860-3.c: New testcase.
	* gcc.dg/pr91860-4.c: New testcase.
---
 gcc/simplify-rtx.c               | 11 +++++++++++
 gcc/testsuite/gcc.dg/pr91860-1.c | 18 ++++++++++++++++++
 gcc/testsuite/gcc.dg/pr91860-2.c | 13 +++++++++++++
 gcc/testsuite/gcc.dg/pr91860-3.c | 15 +++++++++++++++
 gcc/testsuite/gcc.dg/pr91860-4.c | 24 ++++++++++++++++++++++++
 5 files changed, 81 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/pr91860-1.c
 create mode 100644 gcc/testsuite/gcc.dg/pr91860-2.c
 create mode 100644 gcc/testsuite/gcc.dg/pr91860-3.c
 create mode 100644 gcc/testsuite/gcc.dg/pr91860-4.c

diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index 9a70720c764..8593010acf4 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -635,6 +635,17 @@ simplify_truncation (machine_mode mode, rtx op,
 	 is larger than the origmode, we can just extend to the appropriate
 	 mode.  */
       machine_mode origmode = GET_MODE (XEXP (op, 0));
+
+      /* This can happen when called from inside combine, if we have a zero
+	 or sign extend of a CONST_INT.  We assume that all of the bits of the
+	 constant are significant here.  If we don't do this, then we try
+	 to extend VOIDmode, which takes us to simplify_const_unary_operation
+	 which assumes that a VOIDmode operand has the destination mode,
+	 which can then trigger an abort in a wide_int::from call if the
+	 constant isn't already valid for that mode.  */
+      if (origmode == VOIDmode)
+	origmode = MAX_MODE_INT;
+
       if (mode == origmode)
 	return XEXP (op, 0);
       else if (precision <= GET_MODE_UNIT_PRECISION (origmode))
diff --git a/gcc/testsuite/gcc.dg/pr91860-1.c b/gcc/testsuite/gcc.dg/pr91860-1.c
new file mode 100644
index 00000000000..e715040e33d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr91860-1.c
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-Og -fipa-cp -g --param=max-combine-insns=3" } */
+
+char a;
+int b;
+
+static void
+bar (short d)
+{
+  d <<= __builtin_sub_overflow (0, d, &a);
+  b = __builtin_bswap16 (~d);
+}
+
+void
+foo (void)
+{
+  bar (21043);
+}
diff --git a/gcc/testsuite/gcc.dg/pr91860-2.c b/gcc/testsuite/gcc.dg/pr91860-2.c
new file mode 100644
index 00000000000..7b44e648ca6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr91860-2.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-Og -fexpensive-optimizations -fno-tree-fre -g --param=max-combine-insns=4" } */
+
+unsigned a, b, c;
+void
+foo (void)
+{
+  unsigned short e;
+  __builtin_mul_overflow (0, b, &a);
+  __builtin_sub_overflow (59347, 9, &e);
+  e <<= a & 5;
+  c = e;
+}
diff --git a/gcc/testsuite/gcc.dg/pr91860-3.c b/gcc/testsuite/gcc.dg/pr91860-3.c
new file mode 100644
index 00000000000..2b488cc9048
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr91860-3.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-Og -g2 --param=max-combine-insns=3" } */
+
+int a, b;
+
+void
+foo (void)
+{
+  unsigned short d = 46067;
+  int e = e;
+  d <<= __builtin_mul_overflow (~0, e, &a);
+  d |= -68719476735;
+  b = d;
+}
+
diff --git a/gcc/testsuite/gcc.dg/pr91860-4.c b/gcc/testsuite/gcc.dg/pr91860-4.c
new file mode 100644
index 00000000000..36f2bd55c64
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr91860-4.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target int128 } */
+/* { dg-options "-O2 -g" } */
+
+typedef unsigned char u8;
+typedef unsigned int u32;
+typedef unsigned __int128 u128;
+
+u32 b, c;
+
+static inline
+u128 bar (u8 d, u128 e)
+{
+  __builtin_memset (11 + (char *) &e, b, 1);
+  d <<= e & 7;
+  d = d | d > 0;
+  return d + e;
+}
+
+void
+foo (void)
+{
+  c = bar (~0, 5);
+}
-- 
2.17.1

Re: [PATCH] Modify simplify_truncation to handle extended CONST_INT.

[Richard Sandiford]

Jim Wilson <jimw@sifive.com> writes:
> This addresses PR 91860 which has four testcases triggering internal errors.
> The problem here is that in combine when handling debug insns, we are trying
> to substitute
> (sign_extend:DI (const_int 8160 [0x1fe0]))
> as the value for
> (reg:DI 78 [ _9 ])
> in the debug insn
> (debug_insn 29 28 30 2 (var_location:QI d (subreg:QI (reg:DI 78 [ _9 ]) 0)) "tmp4.c":11:5 -1
>      (nil))
>
> The place where this starts to go wrong is in simplify_truncation, where it
> tries to compare the size of the original mode VOIDmode with the subreg mode
> QI and decides that we need to sign extend the constant to convert it from
> VOIDmode to QImode.  We actually need a truncation not a extension here.  Also
> note that the GET_MODE_UNIT_PRECISION (VOIDmode) isn't useful.  We can fix
> this by changing the mode to MAX_MODE_INT, as the CONST_INT should already be
> valid for the largest supported integer mode.  There are already a number of
> other places in simplify-rtx.c that do the same thing.
>
> This was tested with rv32/newlib and rv64/linux cross builds and make checks.
> There were no regressions.  The new tests all fail for rv64 without the patch,
> and work with the patch.

subst tries to avoid creating invalid (zero_extend:DI (const_int N)):

	      else if (CONST_SCALAR_INT_P (new_rtx)
		       && (GET_CODE (x) == ZERO_EXTEND
			   || GET_CODE (x) == FLOAT
			   || GET_CODE (x) == UNSIGNED_FLOAT))

Does adding SIGN_EXTEND to the list fix the bug?

I guess SIGN_EXTEND was excluded here (and in a couple of other places
in combine) on the basis that CONST_INTs are naturally sign-extended,
so the substitution doesn't lose information.  But is a SIGN_EXTEND
of a CONST_INT really valid rtl?  I agree with what you said in the PR
that it shouldn't be, for two reasons:

(1) SIGN_EXTENDs operate on distinct source and destination modes
    (unlike binary operations that operate on one mode).  The natural
    way of getting the source mode is GET_MODE (XEXP (x, 0)), and allowing
    CONST_INTs means that potentially any code processing SIGN_EXTENDs
    would need to check for CONST_INTs before using GET_MODE.

(2) we're still finding cases in which CONST_INTs aren't properly
    canonicalised into sign-extended form.  Allowing SIGN_EXTENDs
    of them turns that from being an internal consistency failure
    to a wrong code bug.

There's also the argument that SIGN_EXTEND and ZERO_EXTEND should
be handled consistently.

Thanks,
Richard

Re: [PATCH] Modify simplify_truncation to handle extended CONST_INT.

[Jim Wilson]

On Thu, Oct 10, 2019 at 1:46 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
> subst tries to avoid creating invalid (zero_extend:DI (const_int N)):
>
>               else if (CONST_SCALAR_INT_P (new_rtx)
>                        && (GET_CODE (x) == ZERO_EXTEND
>                            || GET_CODE (x) == FLOAT
>                            || GET_CODE (x) == UNSIGNED_FLOAT))
>
> Does adding SIGN_EXTEND to the list fix the bug?

I missed that.  I tried that, and it does work.  This looks like a
better solution.  I'm sending a new patch.

Jim

[PATCH, v2] Extend subst to simplify CONST_INT inside SIGN_EXTEND.

[Jim Wilson]

This addresses PR 91860 which has four testcases triggering internal errors.
The problem here is that in combine when handling debug insns, we are trying
to substitute
(sign_extend:DI (const_int 8160 [0x1fe0]))
as the value for
(reg:DI 78 [ _9 ])
in the debug insn
(debug_insn 29 28 30 2 (var_location:QI d (subreg:QI (reg:DI 78 [ _9 ]) 0)) "tmp4.c":11:5 -1
     (nil))
This eventually triggers an abort because 8160 is not a sign-extended
QImode value.

We should avoid creating the invalid RTL in the first place.  In subst there
is already code to avoid putting a CONST_INT inside a ZERO_EXTEND.  This
needs to be extended to also handle a SIGN_EXTEND the same way.

This was tested with rv32/newlib and rv64/linux cross builds and make checks.
There were no regressions.  The new tests all fail for rv64 without the patch,
and work with the patch.

OK?

Jim

	gcc/
	PR rtl-optimization/91860
	* combine.c (subst): If new_rtx is a constant, also check for
	SIGN_EXTEND when deciding whether to call simplify_unary_operation.

	gcc/testsuite/
	PR rtl-optimization/91860
	* gcc.dg/pr91860-1.c: New testcase.
	* gcc.dg/pr91860-2.c: New testcase.
	* gcc.dg/pr91860-3.c: New testcase.
	* gcc.dg/pr91860-4.c: New testcase.
---
 gcc/combine.c                    |  1 +
 gcc/testsuite/gcc.dg/pr91860-1.c | 18 ++++++++++++++++++
 gcc/testsuite/gcc.dg/pr91860-2.c | 13 +++++++++++++
 gcc/testsuite/gcc.dg/pr91860-3.c | 15 +++++++++++++++
 gcc/testsuite/gcc.dg/pr91860-4.c | 24 ++++++++++++++++++++++++
 5 files changed, 71 insertions(+)
 create mode 100644 gcc/testsuite/gcc.dg/pr91860-1.c
 create mode 100644 gcc/testsuite/gcc.dg/pr91860-2.c
 create mode 100644 gcc/testsuite/gcc.dg/pr91860-3.c
 create mode 100644 gcc/testsuite/gcc.dg/pr91860-4.c

diff --git a/gcc/combine.c b/gcc/combine.c
index d295a81abf9..92e4e5e6898 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -5680,6 +5680,7 @@ subst (rtx x, rtx from, rtx to, int in_dest, int in_cond, int unique_copy)
 		}
 	      else if (CONST_SCALAR_INT_P (new_rtx)
 		       && (GET_CODE (x) == ZERO_EXTEND
+			   || GET_CODE (x) == SIGN_EXTEND
 			   || GET_CODE (x) == FLOAT
 			   || GET_CODE (x) == UNSIGNED_FLOAT))
 		{
diff --git a/gcc/testsuite/gcc.dg/pr91860-1.c b/gcc/testsuite/gcc.dg/pr91860-1.c
new file mode 100644
index 00000000000..e715040e33d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr91860-1.c
@@ -0,0 +1,18 @@
+/* { dg-do compile } */
+/* { dg-options "-Og -fipa-cp -g --param=max-combine-insns=3" } */
+
+char a;
+int b;
+
+static void
+bar (short d)
+{
+  d <<= __builtin_sub_overflow (0, d, &a);
+  b = __builtin_bswap16 (~d);
+}
+
+void
+foo (void)
+{
+  bar (21043);
+}
diff --git a/gcc/testsuite/gcc.dg/pr91860-2.c b/gcc/testsuite/gcc.dg/pr91860-2.c
new file mode 100644
index 00000000000..7b44e648ca6
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr91860-2.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options "-Og -fexpensive-optimizations -fno-tree-fre -g --param=max-combine-insns=4" } */
+
+unsigned a, b, c;
+void
+foo (void)
+{
+  unsigned short e;
+  __builtin_mul_overflow (0, b, &a);
+  __builtin_sub_overflow (59347, 9, &e);
+  e <<= a & 5;
+  c = e;
+}
diff --git a/gcc/testsuite/gcc.dg/pr91860-3.c b/gcc/testsuite/gcc.dg/pr91860-3.c
new file mode 100644
index 00000000000..2b488cc9048
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr91860-3.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-Og -g2 --param=max-combine-insns=3" } */
+
+int a, b;
+
+void
+foo (void)
+{
+  unsigned short d = 46067;
+  int e = e;
+  d <<= __builtin_mul_overflow (~0, e, &a);
+  d |= -68719476735;
+  b = d;
+}
+
diff --git a/gcc/testsuite/gcc.dg/pr91860-4.c b/gcc/testsuite/gcc.dg/pr91860-4.c
new file mode 100644
index 00000000000..36f2bd55c64
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr91860-4.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target int128 } */
+/* { dg-options "-O2 -g" } */
+
+typedef unsigned char u8;
+typedef unsigned int u32;
+typedef unsigned __int128 u128;
+
+u32 b, c;
+
+static inline
+u128 bar (u8 d, u128 e)
+{
+  __builtin_memset (11 + (char *) &e, b, 1);
+  d <<= e & 7;
+  d = d | d > 0;
+  return d + e;
+}
+
+void
+foo (void)
+{
+  c = bar (~0, 5);
+}
-- 
2.17.1

Re: [PATCH, v2] Extend subst to simplify CONST_INT inside SIGN_EXTEND.

[Segher Boessenkool]

Hi!

On Thu, Oct 10, 2019 at 03:16:36PM -0700, Jim Wilson wrote:
> This addresses PR 91860 which has four testcases triggering internal errors.
> The problem here is that in combine when handling debug insns, we are trying
> to substitute
> (sign_extend:DI (const_int 8160 [0x1fe0]))
> as the value for
> (reg:DI 78 [ _9 ])
> in the debug insn
> (debug_insn 29 28 30 2 (var_location:QI d (subreg:QI (reg:DI 78 [ _9 ]) 0)) "tmp4.c":11:5 -1
>      (nil))
> This eventually triggers an abort because 8160 is not a sign-extended
> QImode value.
> 
> We should avoid creating the invalid RTL in the first place.

It is *normal* for combine to create invalid RTL.  It first creates it,
and it checks if it is valid later.

However:

> In subst there
> is already code to avoid putting a CONST_INT inside a ZERO_EXTEND.  This
> needs to be extended to also handle a SIGN_EXTEND the same way.

That works, sure.  Approved for trunk.  Thanks!


Segher