| Message ID | 20140609145256.GA10949@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On 06/10/14 01:42, Ilya Enkovich wrote: > Hi, > > This patch fixes PR61446. The problem appears when we insert value copies after transformations. We use the widest extension mode met in a chain, but it may be wider than original destination register size. This patch checks it and use smaller mode if required. > > Bootstrapped and tested on linux-x86_64. > > Does it look OK? > > Thanks, > Ilya > -- > 2014-06-09 Ilya Enkovich <ilya.enkovich@intel.com> > > PR 61446 > * ree.c (find_and_remove_re): Narrow mode for register copy > if required. That seems wrong. Something should have rejected this earlier. Let me take a looksie. jeff
On 06/10/14 01:42, Ilya Enkovich wrote: > Hi, > > This patch fixes PR61446. The problem appears when we insert value copies after transformations. We use the widest extension mode met in a chain, but it may be wider than original destination register size. This patch checks it and use smaller mode if required. > > Bootstrapped and tested on linux-x86_64. > > Does it look OK? > > Thanks, > Ilya > -- > 2014-06-09 Ilya Enkovich <ilya.enkovich@intel.com> > > PR 61446 > * ree.c (find_and_remove_re): Narrow mode for register copy > if required. The whole point behind the 61094 change was to avoid this kind of issue. ie, before eliminating an extension which requires a copy, make sure the copy is going to be valid (single insn that is recognizable and satisfies its constraints). If the copy is not going to be valid, then suppress the extension elimination. It's not working as desired because of a relatively simple goof. When I wrote the changes for 61094, I copied the code which created the new insns from find_and_remove_re into combine_reaching_defs -- the idea being we want to generate the same insn in combine_reaching_defs that will be generated in find_and_remove_re. In combine_reaching_defs we generate, validate & throw it away. In find_and_remove_re we generate and insert it into the insn stream. The subtle issue missed as that in find_and_remove_re, we have already transformed the defining insn. ie, the destination of the defining insn is in the widened mode. That is _not_ the case in combine_reaching_defs. So combine_reaching_defs is not testing the same insn that will be created by find_and_remove_re. The insns have the same structure, but the modes of the operands are different. For 61094, that little difference was not important. It *is* important for 61446. Thankfully the fix is trivial and I've confirmed that 61094 stays fixed and that it fixes 61446. Going through the bootstrap & regression process now. Jeff
diff --git a/gcc/ChangeLog.pr61446 b/gcc/ChangeLog.pr61446 new file mode 100644 index 0000000..b9e2148 --- /dev/null +++ b/gcc/ChangeLog.pr61446 @@ -0,0 +1,5 @@ +2014-06-09 Ilya Enkovich <ilya.enkovich@intel.com> + + PR 61446 + * ree.c (find_and_remove_re): Narrow mode for register copy + if required. diff --git a/gcc/ree.c b/gcc/ree.c index ade413e..6d34764 100644 --- a/gcc/ree.c +++ b/gcc/ree.c @@ -1088,14 +1088,24 @@ find_and_remove_re (void) /* Use the mode of the destination of the defining insn for the mode of the copy. This is necessary if the defining insn was used to eliminate a second extension - that was wider than the first. */ + that was wider than the first. Truncate mode if it is + too wide for destination reg. */ rtx sub_rtx = *get_sub_rtx (def_insn); rtx pat = PATTERN (curr_insn); - rtx new_dst = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)), - REGNO (XEXP (SET_SRC (pat), 0))); - rtx new_src = gen_rtx_REG (GET_MODE (SET_DEST (sub_rtx)), - REGNO (SET_DEST (pat))); - rtx set = gen_rtx_SET (VOIDmode, new_dst, new_src); + unsigned int regno = REGNO (XEXP (SET_SRC (pat), 0)); + enum machine_mode mode = GET_MODE (SET_DEST (sub_rtx)); + rtx new_dst, new_src, set; + + if (HARD_REGNO_NREGS (regno, mode) != 1) + { + mode = GET_CLASS_NARROWEST_MODE (GET_MODE_CLASS (mode)); + while (HARD_REGNO_NREGS (regno, GET_MODE_WIDER_MODE (mode)) == 1) + mode = GET_MODE_WIDER_MODE (mode); + } + + new_dst = gen_rtx_REG (mode, REGNO (XEXP (SET_SRC (pat), 0))); + new_src = gen_rtx_REG (mode, REGNO (SET_DEST (pat))); + set = gen_rtx_SET (VOIDmode, new_dst, new_src); emit_insn_after (set, def_insn); }