| Message ID | 20191009201114.22083-1-jimw@sifive.com |
|---|---|
| State | New |
| Headers | show |
| Series | Modify simplify_truncation to handle extended CONST_INT. | expand |
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
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
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); +}
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