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 <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.
> OK.
>
> Branch maintainers call on how they want to deal with this in 4.8.
>
> jeff
