Patchwork Fix up cmove expansion (PR target/58864, take 2)

login
register
mail settings
Submitter Jakub Jelinek
Date Dec. 2, 2013, 10:51 p.m.
Message ID <20131202225124.GS892@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/296028/
State New
Headers show

Comments

Jakub Jelinek - Dec. 2, 2013, 10:51 p.m.
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.



	Jakub
Jeff Law - Dec. 3, 2013, 6:59 a.m.
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
Richard Guenther - Dec. 3, 2013, 9:05 a.m.
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

Patch

--- 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;
+    }
+}