Re: [PATCH] Fix up cmove expansion (PR target/58864, take 2)

[Andrew Pinski <pinskia at gmail dot com>]

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