Jakub Jelinek <ja...@redhat.com> wrote:
>Hi!
>
>The following testcase ICEs because expand_cond_expr_using_cmove
>calls emit_conditional_move (which calls do_pending_stack_adjust
>under some circumstances), but when that fails, just removes all the
>insns generated by emit_conditional_move (and perhaps some earlier ones
>too), thus it removes also the stack adjustment.
>
>Apparently 2 similar places were fixing it by just calling
>do_pending_stack_adjust () first just in case, some other places
>had (most likely) the same bug as this function.
>
>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.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
>trunk/4.8?
The idea is good but I'd like to see a struct rather than an array for the
storage.
Thanks,
Richard.
>2013-11-29 Jakub Jelinek <ja...@redhat.com>
>
> PR target/58864
> * dojump.c (save_pending_stack_adjust, restore_pending_stack_adjust):
> New functions.
> * expr.h (save_pending_stack_adjust, restore_pending_stack_adjust):
> New prototypes.
> * expr.c (expand_cond_expr_using_cmove): Use it.
> (expand_expr_real_2): Use it instead of unconditional
> do_pending_stack_adjust.
> * optabs.c (expand_doubleword_shift): Use it.
> * expmed.c (expand_sdiv_pow2): Use it instead of unconditional
> do_pending_stack_adjust.
> (emit_store_flag): Use it.
>
> * g++.dg/opt/pr58864.C: New test.
>
>--- gcc/expr.c.jj 2013-11-27 18:02:46.000000000 +0100
>+++ gcc/expr.c 2013-11-29 14:35:12.234808484 +0100
>@@ -7951,6 +7951,9 @@ expand_cond_expr_using_cmove (tree treeo
> else
> temp = assign_temp (type, 0, 1);
>
>+ int save[2];
>+ save_pending_stack_adjust (save);
>+
> start_sequence ();
> expand_operands (treeop1, treeop2,
> temp, &op1, &op2, EXPAND_NORMAL);
>@@ -8009,6 +8012,7 @@ expand_cond_expr_using_cmove (tree treeo
> /* Otherwise discard the sequence and fall back to code with
> branches. */
> end_sequence ();
>+ restore_pending_stack_adjust (save);
> #endif
> return NULL_RTX;
> }
>@@ -8789,12 +8793,9 @@ expand_expr_real_2 (sepops ops, rtx targ
> if (can_conditionally_move_p (mode))
> {
> rtx insn;
>+ int save[2];
>
>- /* ??? 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 ();
>+ save_pending_stack_adjust (save);
>
> start_sequence ();
>
>@@ -8817,6 +8818,7 @@ expand_expr_real_2 (sepops ops, rtx targ
> /* Otherwise discard the sequence and fall back to code with
> branches. */
> end_sequence ();
>+ restore_pending_stack_adjust (save);
> }
> #endif
> if (target != op0)
>--- gcc/optabs.c.jj 2013-11-19 21:56:22.000000000 +0100
>+++ gcc/optabs.c 2013-11-29 14:39:15.963513835 +0100
>@@ -1079,17 +1079,20 @@ expand_doubleword_shift (enum machine_mo
>
> #ifdef HAVE_conditional_move
> /* Try using conditional moves to generate straight-line code. */
>- {
>- rtx start = get_last_insn ();
>- if (expand_doubleword_shift_condmove (op1_mode, binoptab,
>- cmp_code, cmp1, cmp2,
>- outof_input, into_input,
>- op1, superword_op1,
>- outof_target, into_target,
>- unsignedp, methods, shift_mask))
>- return true;
>- delete_insns_since (start);
>- }
>+ int save[2];
>+
>+ save_pending_stack_adjust (save);
>+
>+ rtx start = get_last_insn ();
>+ if (expand_doubleword_shift_condmove (op1_mode, binoptab,
>+ cmp_code, cmp1, cmp2,
>+ outof_input, into_input,
>+ op1, superword_op1,
>+ outof_target, into_target,
>+ unsignedp, methods, shift_mask))
>+ return true;
>+ delete_insns_since (start);
>+ restore_pending_stack_adjust (save);
> #endif
>
>/* As a last resort, use branches to select the correct alternative.
>*/
>--- gcc/dojump.c.jj 2013-11-19 21:56:27.000000000 +0100
>+++ gcc/dojump.c 2013-11-29 14:35:35.088685749 +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 (int save[2])
>+{
>+ save[0] = pending_stack_adjust;
>+ save[1] = stack_pointer_delta;
>+}
>+
>+/* Restore the saved pending_stack_adjust/stack_pointer_delta. */
>+
>+void
>+restore_pending_stack_adjust (int save[2])
>+{
>+ if (inhibit_defer_pop == 0)
>+ {
>+ pending_stack_adjust = save[0];
>+ stack_pointer_delta = save[1];
>+ }
>+}
> >
> /* Expand conditional expressions. */
>
>--- gcc/expr.h.jj 2013-11-19 21:56:27.000000000 +0100
>+++ gcc/expr.h 2013-11-29 14:35:23.438747999 +0100
>@@ -473,6 +473,16 @@ extern void clear_pending_stack_adjust (
>/* Pop any previously-pushed arguments that have not been popped yet.
>*/
> extern void do_pending_stack_adjust (void);
>
>+/* 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 (int [2]);
>+
>+/* Restore the saved pending_stack_adjust/stack_pointer_delta. */
>+
>+extern void restore_pending_stack_adjust (int [2]);
>+
> /* Return the tree node and offset if a given argument corresponds to
> a string constant. */
> extern tree string_constant (tree, tree *);
>--- gcc/expmed.c.jj 2013-11-19 21:56:36.000000000 +0100
>+++ gcc/expmed.c 2013-11-29 14:41:16.887872205 +0100
>@@ -3735,11 +3735,9 @@ expand_sdiv_pow2 (enum machine_mode mode
> >= 2)
> {
> rtx temp2;
>+ int save[2];
>
>- /* ??? 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 ();
>+ save_pending_stack_adjust (save);
>
> start_sequence ();
> temp2 = copy_to_mode_reg (mode, op0);
>@@ -3758,6 +3756,7 @@ expand_sdiv_pow2 (enum machine_mode mode
> return expand_shift (RSHIFT_EXPR, mode, temp2, logd, NULL_RTX, 0);
> }
> end_sequence ();
>+ restore_pending_stack_adjust (save);
> }
> #endif
>
>@@ -5508,6 +5507,9 @@ emit_store_flag (rtx target, enum rtx_co
> if (tem == 0)
> return 0;
>
>+ int save[2];
>+ save_pending_stack_adjust (save);
>+
> if (and_them)
> tem = emit_conditional_move (target, code, op0, op1, mode,
> tem, const0_rtx, GET_MODE (tem), 0);
>@@ -5516,7 +5518,10 @@ emit_store_flag (rtx target, enum rtx_co
> trueval, tem, GET_MODE (tem), 0);
>
> if (tem == 0)
>- delete_insns_since (last);
>+ {
>+ delete_insns_since (last);
>+ restore_pending_stack_adjust (save);
>+ }
> return tem;
> #else
> return 0;
>--- gcc/testsuite/g++.dg/opt/pr58864.C.jj 2013-11-29 13:49:24.450415660
>+0100
>+++ gcc/testsuite/g++.dg/opt/pr58864.C 2013-11-29 13:47:47.000000000
>+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
