On Mon, Dec 2, 2013 at 2:51 PM, Jakub Jelinek <ja...@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 <ja...@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