| Message ID | 20131202225124.GS892@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 12/02/13 15:51, Jakub Jelinek wrote: > Hi! > > On Sat, Nov 30, 2013 at 12:38:30PM +0100, Eric Botcazou wrote: >>> Rather than adding do_pending_stack_adjust () in all the places, especially >>> when it isn't clear whether emit_conditional_move will be called at all and >>> whether it will actually do do_pending_stack_adjust (), I chose to add >>> two new functions to save/restore the pending stack adjustment state, >>> so that when instruction sequence is thrown away (either by doing >>> start_sequence/end_sequence around it and not emitting it, or >>> delete_insns_since) the state can be restored, and have changed all the >>> places that IMHO need it for emit_conditional_move. >> >> Why not do it in emit_conditional_move directly then? The code thinks it's >> clever to do: >> >> do_pending_stack_adjust (); >> last = get_last_insn (); >> prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1), >> GET_CODE (comparison), NULL_RTX, unsignedp, OPTAB_WIDEN, >> &comparison, &cmode); >> [...] >> delete_insns_since (last); >> return NULL_RTX; >> >> but apparently not, so why not delete the stack adjustment as well and restore >> the state afterwards? > > On Sat, Nov 30, 2013 at 09:10:35AM +0100, Richard Biener wrote: >> The idea is good but I'd like to see a struct rather than an array for the >> storage. > > So, this patch attempts to include both of the proposed changes. > Furthermore, I've noticed that calls.c has been saving/restoring those > two values by hand, so now it can use the new APIs for that too. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > What about 4.8 branch? I could create an alternative patch for 4.8, > keep everything as is and just save/restore the two fields by hand in > emit_conditional_move like calls.c used to do it. > > 2013-12-02 Jakub Jelinek <jakub@redhat.com> > > PR target/58864 > * dojump.c (save_pending_stack_adjust, restore_pending_stack_adjust): > New functions. > * expr.h (struct saved_pending_stack_adjust): New type. > (save_pending_stack_adjust, restore_pending_stack_adjust): New > prototypes. > * optabs.c (emit_conditional_move): Call save_pending_stack_adjust > and get_last_insn before do_pending_stack_adjust, call > restore_pending_stack_adjust after delete_insns_since. > * expr.c (expand_expr_real_2): Don't call do_pending_stack_adjust > before calling emit_conditional_move. > * expmed.c (expand_sdiv_pow2): Likewise. > * calls.c (expand_call): Use {save,restore}_pending_stack_adjust. > > * g++.dg/opt/pr58864.C: New test. OK. Branch maintainers call on how they want to deal with this in 4.8. jeff
On Mon, 2 Dec 2013, Jeff Law wrote: > On 12/02/13 15:51, Jakub Jelinek wrote: > > Hi! > > > > On Sat, Nov 30, 2013 at 12:38:30PM +0100, Eric Botcazou wrote: > > > > Rather than adding do_pending_stack_adjust () in all the places, > > > > especially > > > > when it isn't clear whether emit_conditional_move will be called at all > > > > and > > > > whether it will actually do do_pending_stack_adjust (), I chose to add > > > > two new functions to save/restore the pending stack adjustment state, > > > > so that when instruction sequence is thrown away (either by doing > > > > start_sequence/end_sequence around it and not emitting it, or > > > > delete_insns_since) the state can be restored, and have changed all the > > > > places that IMHO need it for emit_conditional_move. > > > > > > Why not do it in emit_conditional_move directly then? The code thinks > > > it's > > > clever to do: > > > > > > do_pending_stack_adjust (); > > > last = get_last_insn (); > > > prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1), > > > GET_CODE (comparison), NULL_RTX, unsignedp, OPTAB_WIDEN, > > > &comparison, &cmode); > > > [...] > > > delete_insns_since (last); > > > return NULL_RTX; > > > > > > but apparently not, so why not delete the stack adjustment as well and > > > restore > > > the state afterwards? > > > > On Sat, Nov 30, 2013 at 09:10:35AM +0100, Richard Biener wrote: > > > The idea is good but I'd like to see a struct rather than an array for the > > > storage. > > > > So, this patch attempts to include both of the proposed changes. > > Furthermore, I've noticed that calls.c has been saving/restoring those > > two values by hand, so now it can use the new APIs for that too. > > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > > > What about 4.8 branch? I could create an alternative patch for 4.8, > > keep everything as is and just save/restore the two fields by hand in > > emit_conditional_move like calls.c used to do it. The patch is ok for the branch as-is after a while on trunk. Thanks, Richard. > > 2013-12-02 Jakub Jelinek <jakub@redhat.com> > > > > PR target/58864 > > * dojump.c (save_pending_stack_adjust, restore_pending_stack_adjust): > > New functions. > > * expr.h (struct saved_pending_stack_adjust): New type. > > (save_pending_stack_adjust, restore_pending_stack_adjust): New > > prototypes. > > * optabs.c (emit_conditional_move): Call save_pending_stack_adjust > > and get_last_insn before do_pending_stack_adjust, call > > restore_pending_stack_adjust after delete_insns_since. > > * expr.c (expand_expr_real_2): Don't call do_pending_stack_adjust > > before calling emit_conditional_move. > > * expmed.c (expand_sdiv_pow2): Likewise. > > * calls.c (expand_call): Use {save,restore}_pending_stack_adjust. > > > > * g++.dg/opt/pr58864.C: New test. > OK. > > Branch maintainers call on how they want to deal with this in 4.8. > > jeff
On Mon, Dec 2, 2013 at 2:51 PM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > On Sat, Nov 30, 2013 at 12:38:30PM +0100, Eric Botcazou wrote: >> > Rather than adding do_pending_stack_adjust () in all the places, especially >> > when it isn't clear whether emit_conditional_move will be called at all and >> > whether it will actually do do_pending_stack_adjust (), I chose to add >> > two new functions to save/restore the pending stack adjustment state, >> > so that when instruction sequence is thrown away (either by doing >> > start_sequence/end_sequence around it and not emitting it, or >> > delete_insns_since) the state can be restored, and have changed all the >> > places that IMHO need it for emit_conditional_move. >> >> Why not do it in emit_conditional_move directly then? The code thinks it's >> clever to do: >> >> do_pending_stack_adjust (); >> last = get_last_insn (); >> prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1), >> GET_CODE (comparison), NULL_RTX, unsignedp, OPTAB_WIDEN, >> &comparison, &cmode); >> [...] >> delete_insns_since (last); >> return NULL_RTX; >> >> but apparently not, so why not delete the stack adjustment as well and restore >> the state afterwards? > > On Sat, Nov 30, 2013 at 09:10:35AM +0100, Richard Biener wrote: >> The idea is good but I'd like to see a struct rather than an array for the >> storage. > > So, this patch attempts to include both of the proposed changes. > Furthermore, I've noticed that calls.c has been saving/restoring those > two values by hand, so now it can use the new APIs for that too. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Note a similar patch is also needed for emit_conditional_add . I will submit it after I test it on both the trunk and with some changes I had to use emit_conditional_add in expand time. Thanks, Andrew > > What about 4.8 branch? I could create an alternative patch for 4.8, > keep everything as is and just save/restore the two fields by hand in > emit_conditional_move like calls.c used to do it. > > 2013-12-02 Jakub Jelinek <jakub@redhat.com> > > PR target/58864 > * dojump.c (save_pending_stack_adjust, restore_pending_stack_adjust): > New functions. > * expr.h (struct saved_pending_stack_adjust): New type. > (save_pending_stack_adjust, restore_pending_stack_adjust): New > prototypes. > * optabs.c (emit_conditional_move): Call save_pending_stack_adjust > and get_last_insn before do_pending_stack_adjust, call > restore_pending_stack_adjust after delete_insns_since. > * expr.c (expand_expr_real_2): Don't call do_pending_stack_adjust > before calling emit_conditional_move. > * expmed.c (expand_sdiv_pow2): Likewise. > * calls.c (expand_call): Use {save,restore}_pending_stack_adjust. > > * g++.dg/opt/pr58864.C: New test. > > --- gcc/dojump.c.jj 2013-12-02 14:33:25.954413998 +0100 > +++ gcc/dojump.c 2013-12-02 15:08:39.958423641 +0100 > @@ -96,6 +96,29 @@ do_pending_stack_adjust (void) > pending_stack_adjust = 0; > } > } > + > +/* Remember pending_stack_adjust/stack_pointer_delta. > + To be used around code that may call do_pending_stack_adjust (), > + but the generated code could be discarded e.g. using delete_insns_since. */ > + > +void > +save_pending_stack_adjust (saved_pending_stack_adjust *save) > +{ > + save->x_pending_stack_adjust = pending_stack_adjust; > + save->x_stack_pointer_delta = stack_pointer_delta; > +} > + > +/* Restore the saved pending_stack_adjust/stack_pointer_delta. */ > + > +void > +restore_pending_stack_adjust (saved_pending_stack_adjust *save) > +{ > + if (inhibit_defer_pop == 0) > + { > + pending_stack_adjust = save->x_pending_stack_adjust; > + stack_pointer_delta = save->x_stack_pointer_delta; > + } > +} > > /* Expand conditional expressions. */ > > --- gcc/expr.h.jj 2013-12-02 14:33:26.263412414 +0100 > +++ gcc/expr.h 2013-12-02 14:50:15.374175604 +0100 > @@ -473,6 +473,28 @@ extern void clear_pending_stack_adjust ( > /* Pop any previously-pushed arguments that have not been popped yet. */ > extern void do_pending_stack_adjust (void); > > +/* Struct for saving/restoring of pending_stack_adjust/stack_pointer_delta > + values. */ > + > +struct saved_pending_stack_adjust > +{ > + /* Saved value of pending_stack_adjust. */ > + int x_pending_stack_adjust; > + > + /* Saved value of stack_pointer_delta. */ > + int x_stack_pointer_delta; > +}; > + > +/* Remember pending_stack_adjust/stack_pointer_delta. > + To be used around code that may call do_pending_stack_adjust (), > + but the generated code could be discarded e.g. using delete_insns_since. */ > + > +extern void save_pending_stack_adjust (saved_pending_stack_adjust *); > + > +/* Restore the saved pending_stack_adjust/stack_pointer_delta. */ > + > +extern void restore_pending_stack_adjust (saved_pending_stack_adjust *); > + > /* Return the tree node and offset if a given argument corresponds to > a string constant. */ > extern tree string_constant (tree, tree *); > --- gcc/optabs.c.jj 2013-12-02 14:33:25.000000000 +0100 > +++ gcc/optabs.c 2013-12-02 15:12:04.000000000 +0100 > @@ -4566,8 +4566,10 @@ emit_conditional_move (rtx target, enum > if (!COMPARISON_P (comparison)) > return NULL_RTX; > > - do_pending_stack_adjust (); > + saved_pending_stack_adjust save; > + save_pending_stack_adjust (&save); > last = get_last_insn (); > + do_pending_stack_adjust (); > prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1), > GET_CODE (comparison), NULL_RTX, unsignedp, OPTAB_WIDEN, > &comparison, &cmode); > @@ -4587,6 +4589,7 @@ emit_conditional_move (rtx target, enum > } > } > delete_insns_since (last); > + restore_pending_stack_adjust (&save); > return NULL_RTX; > } > > --- gcc/expr.c.jj 2013-12-02 14:33:33.047377131 +0100 > +++ gcc/expr.c 2013-12-02 15:10:40.511794869 +0100 > @@ -8790,12 +8790,6 @@ expand_expr_real_2 (sepops ops, rtx targ > { > rtx insn; > > - /* ??? Same problem as in expmed.c: emit_conditional_move > - forces a stack adjustment via compare_from_rtx, and we > - lose the stack adjustment if the sequence we are about > - to create is discarded. */ > - do_pending_stack_adjust (); > - > start_sequence (); > > /* Try to emit the conditional move. */ > --- gcc/expmed.c.jj 2013-12-02 14:33:26.156412923 +0100 > +++ gcc/expmed.c 2013-12-02 15:10:18.606894232 +0100 > @@ -3736,11 +3736,6 @@ expand_sdiv_pow2 (enum machine_mode mode > { > rtx temp2; > > - /* ??? emit_conditional_move forces a stack adjustment via > - compare_from_rtx so, if the sequence is discarded, it will > - be lost. Do it now instead. */ > - do_pending_stack_adjust (); > - > start_sequence (); > temp2 = copy_to_mode_reg (mode, op0); > temp = expand_binop (mode, add_optab, temp2, gen_int_mode (d - 1, mode), > --- gcc/calls.c.jj 2013-11-22 21:03:14.000000000 +0100 > +++ gcc/calls.c 2013-12-02 15:25:37.537126526 +0100 > @@ -2672,8 +2672,7 @@ expand_call (tree exp, rtx target, int i > recursion "call". That way we know any adjustment after the tail > recursion call can be ignored if we indeed use the tail > call expansion. */ > - int save_pending_stack_adjust = 0; > - int save_stack_pointer_delta = 0; > + saved_pending_stack_adjust save; > rtx insns; > rtx before_call, next_arg_reg, after_args; > > @@ -2681,8 +2680,7 @@ expand_call (tree exp, rtx target, int i > { > /* State variables we need to save and restore between > iterations. */ > - save_pending_stack_adjust = pending_stack_adjust; > - save_stack_pointer_delta = stack_pointer_delta; > + save_pending_stack_adjust (&save); > } > if (pass) > flags &= ~ECF_SIBCALL; > @@ -3438,8 +3436,7 @@ expand_call (tree exp, rtx target, int i > /* Restore the pending stack adjustment now that we have > finished generating the sibling call sequence. */ > > - pending_stack_adjust = save_pending_stack_adjust; > - stack_pointer_delta = save_stack_pointer_delta; > + restore_pending_stack_adjust (&save); > > /* Prepare arg structure for next iteration. */ > for (i = 0; i < num_actuals; i++) > --- gcc/testsuite/g++.dg/opt/pr58864.C.jj 2013-12-02 14:47:16.212105758 +0100 > +++ gcc/testsuite/g++.dg/opt/pr58864.C 2013-12-02 14:47:16.212105758 +0100 > @@ -0,0 +1,21 @@ > +// PR target/58864 > +// { dg-do compile } > +// { dg-options "-Os" } > +// { dg-additional-options "-march=i686" { target { { i?86-*-* x86_64-*-* } && ia32 } } } > + > +struct A { A (); ~A (); }; > +struct B { B (); }; > + > +float d, e; > + > +void > +foo () > +{ > + A a; > + float c = d; > + while (1) > + { > + B b; > + e = c ? -c : 0; > + } > +} > > > Jakub
--- gcc/dojump.c.jj 2013-12-02 14:33:25.954413998 +0100 +++ gcc/dojump.c 2013-12-02 15:08:39.958423641 +0100 @@ -96,6 +96,29 @@ do_pending_stack_adjust (void) pending_stack_adjust = 0; } } + +/* Remember pending_stack_adjust/stack_pointer_delta. + To be used around code that may call do_pending_stack_adjust (), + but the generated code could be discarded e.g. using delete_insns_since. */ + +void +save_pending_stack_adjust (saved_pending_stack_adjust *save) +{ + save->x_pending_stack_adjust = pending_stack_adjust; + save->x_stack_pointer_delta = stack_pointer_delta; +} + +/* Restore the saved pending_stack_adjust/stack_pointer_delta. */ + +void +restore_pending_stack_adjust (saved_pending_stack_adjust *save) +{ + if (inhibit_defer_pop == 0) + { + pending_stack_adjust = save->x_pending_stack_adjust; + stack_pointer_delta = save->x_stack_pointer_delta; + } +} /* Expand conditional expressions. */ --- gcc/expr.h.jj 2013-12-02 14:33:26.263412414 +0100 +++ gcc/expr.h 2013-12-02 14:50:15.374175604 +0100 @@ -473,6 +473,28 @@ extern void clear_pending_stack_adjust ( /* Pop any previously-pushed arguments that have not been popped yet. */ extern void do_pending_stack_adjust (void); +/* Struct for saving/restoring of pending_stack_adjust/stack_pointer_delta + values. */ + +struct saved_pending_stack_adjust +{ + /* Saved value of pending_stack_adjust. */ + int x_pending_stack_adjust; + + /* Saved value of stack_pointer_delta. */ + int x_stack_pointer_delta; +}; + +/* Remember pending_stack_adjust/stack_pointer_delta. + To be used around code that may call do_pending_stack_adjust (), + but the generated code could be discarded e.g. using delete_insns_since. */ + +extern void save_pending_stack_adjust (saved_pending_stack_adjust *); + +/* Restore the saved pending_stack_adjust/stack_pointer_delta. */ + +extern void restore_pending_stack_adjust (saved_pending_stack_adjust *); + /* Return the tree node and offset if a given argument corresponds to a string constant. */ extern tree string_constant (tree, tree *); --- gcc/optabs.c.jj 2013-12-02 14:33:25.000000000 +0100 +++ gcc/optabs.c 2013-12-02 15:12:04.000000000 +0100 @@ -4566,8 +4566,10 @@ emit_conditional_move (rtx target, enum if (!COMPARISON_P (comparison)) return NULL_RTX; - do_pending_stack_adjust (); + saved_pending_stack_adjust save; + save_pending_stack_adjust (&save); last = get_last_insn (); + do_pending_stack_adjust (); prepare_cmp_insn (XEXP (comparison, 0), XEXP (comparison, 1), GET_CODE (comparison), NULL_RTX, unsignedp, OPTAB_WIDEN, &comparison, &cmode); @@ -4587,6 +4589,7 @@ emit_conditional_move (rtx target, enum } } delete_insns_since (last); + restore_pending_stack_adjust (&save); return NULL_RTX; } --- gcc/expr.c.jj 2013-12-02 14:33:33.047377131 +0100 +++ gcc/expr.c 2013-12-02 15:10:40.511794869 +0100 @@ -8790,12 +8790,6 @@ expand_expr_real_2 (sepops ops, rtx targ { rtx insn; - /* ??? Same problem as in expmed.c: emit_conditional_move - forces a stack adjustment via compare_from_rtx, and we - lose the stack adjustment if the sequence we are about - to create is discarded. */ - do_pending_stack_adjust (); - start_sequence (); /* Try to emit the conditional move. */ --- gcc/expmed.c.jj 2013-12-02 14:33:26.156412923 +0100 +++ gcc/expmed.c 2013-12-02 15:10:18.606894232 +0100 @@ -3736,11 +3736,6 @@ expand_sdiv_pow2 (enum machine_mode mode { rtx temp2; - /* ??? emit_conditional_move forces a stack adjustment via - compare_from_rtx so, if the sequence is discarded, it will - be lost. Do it now instead. */ - do_pending_stack_adjust (); - start_sequence (); temp2 = copy_to_mode_reg (mode, op0); temp = expand_binop (mode, add_optab, temp2, gen_int_mode (d - 1, mode), --- gcc/calls.c.jj 2013-11-22 21:03:14.000000000 +0100 +++ gcc/calls.c 2013-12-02 15:25:37.537126526 +0100 @@ -2672,8 +2672,7 @@ expand_call (tree exp, rtx target, int i recursion "call". That way we know any adjustment after the tail recursion call can be ignored if we indeed use the tail call expansion. */ - int save_pending_stack_adjust = 0; - int save_stack_pointer_delta = 0; + saved_pending_stack_adjust save; rtx insns; rtx before_call, next_arg_reg, after_args; @@ -2681,8 +2680,7 @@ expand_call (tree exp, rtx target, int i { /* State variables we need to save and restore between iterations. */ - save_pending_stack_adjust = pending_stack_adjust; - save_stack_pointer_delta = stack_pointer_delta; + save_pending_stack_adjust (&save); } if (pass) flags &= ~ECF_SIBCALL; @@ -3438,8 +3436,7 @@ expand_call (tree exp, rtx target, int i /* Restore the pending stack adjustment now that we have finished generating the sibling call sequence. */ - pending_stack_adjust = save_pending_stack_adjust; - stack_pointer_delta = save_stack_pointer_delta; + restore_pending_stack_adjust (&save); /* Prepare arg structure for next iteration. */ for (i = 0; i < num_actuals; i++) --- gcc/testsuite/g++.dg/opt/pr58864.C.jj 2013-12-02 14:47:16.212105758 +0100 +++ gcc/testsuite/g++.dg/opt/pr58864.C 2013-12-02 14:47:16.212105758 +0100 @@ -0,0 +1,21 @@ +// PR target/58864 +// { dg-do compile } +// { dg-options "-Os" } +// { dg-additional-options "-march=i686" { target { { i?86-*-* x86_64-*-* } && ia32 } } } + +struct A { A (); ~A (); }; +struct B { B (); }; + +float d, e; + +void +foo () +{ + A a; + float c = d; + while (1) + { + B b; + e = c ? -c : 0; + } +}