2014-10-10 21:10 GMT+04:00 Uros Bizjak <ubiz...@gmail.com>: > On Wed, Oct 1, 2014 at 8:57 PM, Vladimir Makarov <vmaka...@redhat.com> wrote: > >> The problem is in code introduced by Bernd in IRA and caller-saves.c in >> 2012. It is basically an optimization for functions returning always the >> same result as one argument (e.g. memcpy returning 1st argument). >> >> There are two possible solutions. First one is to prohibit the >> optimizations when there is a parallel in SET. Second one is to go deeper >> if the call result is guaranteed in the first element which is true for the >> patch. > > I suspect that the first solution will regress > gcc.target/i386/retarg.c on i686 - that testcase test if referred > optimization is effective. All things equal, I think we should go with > the second solution.
The first solutions is in trunk since October 3 (https://gcc.gnu.org/ml/gcc-cvs/2014-10/msg00094.html) and I don't see such fail. Patch actually just checks for a case which never occurs right now and therefore should be quite safe. Ilya > >> For the first solution, the patch would be >> >> Index: lra-constraints.c >> =================================================================== >> --- lra-constraints.c (revision 215748) >> +++ lra-constraints.c (working copy) >> @@ -5348,16 +5348,19 @@ >> if (GET_CODE (pat) == PARALLEL) >> pat = XVECEXP (pat, 0, 0); >> dest = SET_DEST (pat); >> - start_sequence (); >> - emit_move_insn (cheap, copy_rtx (dest)); >> - restore = get_insns (); >> - end_sequence (); >> - lra_process_new_insns (curr_insn, NULL, restore, >> - "Inserting call parameter >> restore"); >> - /* We don't need to save/restore of the pseudo from >> - this call. */ >> - usage_insns[regno].calls_num = calls_num; >> - bitmap_set_bit (&check_only_regs, regno); >> + if (REG_P (dest)) >> + { >> + start_sequence (); >> + emit_move_insn (cheap, copy_rtx (dest)); >> + restore = get_insns (); >> + end_sequence (); >> + lra_process_new_insns (curr_insn, NULL, restore, >> + "Inserting call parameter >> restore"); >> + /* We don't need to save/restore of the pseudo >> + from this call. */ >> + usage_insns[regno].calls_num = calls_num; >> + bitmap_set_bit (&check_only_regs, regno); >> + } >> } >> } >> to_inherit_num = 0; >> >> >> For the second solution, the patch is >> >> >> Index: lra-constraints.c >> =================================================================== >> --- lra-constraints.c (revision 215748) >> +++ lra-constraints.c (working copy) >> @@ -5348,16 +5348,25 @@ >> if (GET_CODE (pat) == PARALLEL) >> pat = XVECEXP (pat, 0, 0); >> dest = SET_DEST (pat); >> - start_sequence (); >> - emit_move_insn (cheap, copy_rtx (dest)); >> - restore = get_insns (); >> - end_sequence (); >> - lra_process_new_insns (curr_insn, NULL, restore, >> - "Inserting call parameter >> restore"); >> - /* We don't need to save/restore of the pseudo from >> - this call. */ >> - usage_insns[regno].calls_num = calls_num; >> - bitmap_set_bit (&check_only_regs, regno); >> + if (GET_CODE (dest) == PARALLEL) >> + { >> + dest = XVECEXP (dest, 0, 0); >> + if (GET_CODE (dest) == EXPR_LIST) >> + dest = XEXP (dest, 0); >> + } >> + if (REG_P (dest)) >> + { >> + start_sequence (); >> + emit_move_insn (cheap, copy_rtx (dest)); >> + restore = get_insns (); >> + end_sequence (); >> + lra_process_new_insns (curr_insn, NULL, restore, >> + "Inserting call parameter >> restore"); >> + /* We don't need to save/restore of the pseudo from >> + this call. */ >> + usage_insns[regno].calls_num = calls_num; >> + bitmap_set_bit (&check_only_regs, regno); >> + } >> } >> } >> >> >> The first patch is safer but the second one is ok too. I have no particular >> preferences. Whatever we choose, analogous code in caller-saves.c should be >> changed too. > > Uros.