Modify simplify_truncation to handle extended CONST_INT.
diff mbox series

Message ID 20191009201114.22083-1-jimw@sifive.com
State New
Headers show
Series
  • Modify simplify_truncation to handle extended CONST_INT.
Related show

Commit Message

Jim Wilson Oct. 9, 2019, 8:11 p.m. UTC
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

Comments

Richard Sandiford Oct. 10, 2019, 8:46 a.m. UTC | #1
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
Jim Wilson Oct. 10, 2019, 9:45 p.m. UTC | #2
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
diff mbox series

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);
+}