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 <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
