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