> Thanks! Jakub noticed a potential problem in this area a while back, > but I never came up with any code to trigger and have kept that issue on > my todo list ever since. > > Rather than ensuring the inserted copy write a single register, it seems > to me we're better off ensuring that the number of hard registers hasn't > changed. Obviously that's not much of an issue for things like aarch64, > x86_64, but for the embedded 32 bit parts it's likely much more important. > > So I think the test is something like > > x = get_extended_src_reg (SET_SRC (PATTERN (cand->insn))) > if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode) > != HARD_REGNO_NREGS (REGNO (new_reg), GET_MODE (SET_DEST (*orig_set)) > return false;
I'm not 100% sure the rest of the code will correctly deal with multiple registers in all possible combinations. I think adding more asserts would help avoid similar silent codegeneration failures. For example, should widening from 1 to 2 registers: (set (reg:TI x2) (zero_extend:TI (reg:DI x2))) be regarded as a "copy_needed"? And what about TI x2 = zero_extend DI x3? Both cases have overlaps, but copy_needed is set differently... I also noticed there are more places where get_extended_src_reg is not used (eg. in the code that emits the final copy). Anyway here is the modified check: --- gcc/ree.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/gcc/ree.c b/gcc/ree.c index 1431b7a..fa40484 100644 --- a/gcc/ree.c +++ b/gcc/ree.c @@ -743,6 +743,14 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state) if (!SCALAR_INT_MODE_P (GET_MODE (SET_DEST (PATTERN (cand->insn))))) return false; + enum machine_mode dst_mode = GET_MODE (SET_DEST (PATTERN (cand->insn))); + rtx src_reg = get_extended_src_reg (SET_SRC (PATTERN (cand->insn))); + + /* Ensure the number of hard registers of the copy match. */ + if (HARD_REGNO_NREGS (REGNO (src_reg), dst_mode) + != HARD_REGNO_NREGS (REGNO (src_reg), GET_MODE (src_reg))) + return false; + /* There's only one reaching def. */ rtx_insn *def_insn = state->defs_list[0]; @@ -792,7 +800,7 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state) start_sequence (); rtx pat = PATTERN (cand->insn); rtx new_dst = gen_rtx_REG (GET_MODE (SET_DEST (pat)), - REGNO (XEXP (SET_SRC (pat), 0))); + REGNO (get_extended_src_reg (SET_SRC (pat)))); rtx new_src = gen_rtx_REG (GET_MODE (SET_DEST (pat)), REGNO (SET_DEST (pat))); emit_move_insn (new_dst, new_src); -- 1.9.1