Fix up cmove expansion (PR target/58864)

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.


	Jakub

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];
>+    }
>+}
> >
> /* 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

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