| Message ID | 542C4E8F.8030502@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Oct 1, 2014 at 8:57 PM, Vladimir Makarov <vmakarov@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. > 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.
2014-10-10 21:10 GMT+04:00 Uros Bizjak <ubizjak@gmail.com>: > On Wed, Oct 1, 2014 at 8:57 PM, Vladimir Makarov <vmakarov@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.
On Fri, Oct 10, 2014 at 7:29 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2014-10-10 21:10 GMT+04:00 Uros Bizjak <ubizjak@gmail.com>: >> On Wed, Oct 1, 2014 at 8:57 PM, Vladimir Makarov <vmakarov@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. True, but after MPX patches are committed, PARALLELs will be passed as call targets. I wonder if the testcase fails then. Uros.
On 10/10/14 11:33, Uros Bizjak wrote: > On Fri, Oct 10, 2014 at 7:29 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> 2014-10-10 21:10 GMT+04:00 Uros Bizjak <ubizjak@gmail.com>: >>> On Wed, Oct 1, 2014 at 8:57 PM, Vladimir Makarov <vmakarov@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. > > True, but after MPX patches are committed, PARALLELs will be passed as > call targets. I wonder if the testcase fails then. Well, before those patches can be committed, they have to survive bootstrap and regression testing as a whole :-) jeff
2014-10-10 21:35 GMT+04:00 Jeff Law <law@redhat.com>: > On 10/10/14 11:33, Uros Bizjak wrote: >> >> On Fri, Oct 10, 2014 at 7:29 PM, Ilya Enkovich <enkovich.gnu@gmail.com> >> wrote: >>> >>> 2014-10-10 21:10 GMT+04:00 Uros Bizjak <ubizjak@gmail.com>: >>>> >>>> On Wed, Oct 1, 2014 at 8:57 PM, Vladimir Makarov <vmakarov@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. >> >> >> True, but after MPX patches are committed, PARALLELs will be passed as >> call targets. I wonder if the testcase fails then. > > Well, before those patches can be committed, they have to survive bootstrap > and regression testing as a whole :-) Surely! Also PARALLEL will not be passed until you pass -mmpx flag. Ilya > > jeff
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); + } }