Jim Wilson <j...@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
