* [PATCH] Fix up cmove expansion (PR target/58864)
@ 2013-11-30 17:55 Jakub Jelinek
2013-11-30 18:16 ` Richard Biener
[not found] ` <1590476.2x66Irb6tH@polaris>
0 siblings, 2 replies; 6+ messages in thread
From: Jakub Jelinek @ 2013-11-30 17:55 UTC (permalink / raw)
To: Eric Botcazou, Steven Bosscher, Richard Biener; +Cc: gcc-patches
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?
2013-11-29 Jakub Jelinek <jakub@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];
+ }
+}
\f
/* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix up cmove expansion (PR target/58864)
2013-11-30 17:55 [PATCH] Fix up cmove expansion (PR target/58864) Jakub Jelinek
@ 2013-11-30 18:16 ` Richard Biener
[not found] ` <1590476.2x66Irb6tH@polaris>
1 sibling, 0 replies; 6+ messages in thread
From: Richard Biener @ 2013-11-30 18:16 UTC (permalink / raw)
To: Jakub Jelinek, Jakub Jelinek, Eric Botcazou, Steven Bosscher; +Cc: gcc-patches
Jakub Jelinek <jakub@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 <jakub@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];
>+ }
>+}
> \f>
> /* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Fix up cmove expansion (PR target/58864, take 2)
[not found] ` <1590476.2x66Irb6tH@polaris>
@ 2013-12-02 22:51 ` Jakub Jelinek
2013-12-03 6:59 ` Jeff Law
2014-11-08 4:55 ` Andrew Pinski
0 siblings, 2 replies; 6+ messages in thread
From: Jakub Jelinek @ 2013-12-02 22:51 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches, Steven Bosscher, Richard Biener
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.
--- 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;
+ }
+}
\f
/* 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix up cmove expansion (PR target/58864, take 2)
2013-12-02 22:51 ` [PATCH] Fix up cmove expansion (PR target/58864, take 2) Jakub Jelinek
@ 2013-12-03 6:59 ` Jeff Law
2013-12-03 9:05 ` Richard Biener
2014-11-08 4:55 ` Andrew Pinski
1 sibling, 1 reply; 6+ messages in thread
From: Jeff Law @ 2013-12-03 6:59 UTC (permalink / raw)
To: Jakub Jelinek, Eric Botcazou; +Cc: gcc-patches, Steven Bosscher, Richard Biener
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix up cmove expansion (PR target/58864, take 2)
2013-12-03 6:59 ` Jeff Law
@ 2013-12-03 9:05 ` Richard Biener
0 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2013-12-03 9:05 UTC (permalink / raw)
To: Jeff Law; +Cc: Jakub Jelinek, Eric Botcazou, gcc-patches, Steven Bosscher
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix up cmove expansion (PR target/58864, take 2)
2013-12-02 22:51 ` [PATCH] Fix up cmove expansion (PR target/58864, take 2) Jakub Jelinek
2013-12-03 6:59 ` Jeff Law
@ 2014-11-08 4:55 ` Andrew Pinski
1 sibling, 0 replies; 6+ messages in thread
From: Andrew Pinski @ 2014-11-08 4:55 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Eric Botcazou, GCC Patches, Steven Bosscher, Richard Biener
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-11-08 4:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-30 17:55 [PATCH] Fix up cmove expansion (PR target/58864) Jakub Jelinek
2013-11-30 18:16 ` Richard Biener
[not found] ` <1590476.2x66Irb6tH@polaris>
2013-12-02 22:51 ` [PATCH] Fix up cmove expansion (PR target/58864, take 2) Jakub Jelinek
2013-12-03 6:59 ` Jeff Law
2013-12-03 9:05 ` Richard Biener
2014-11-08 4:55 ` Andrew Pinski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).