From patchwork Mon Dec 2 22:51:25 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 296028 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 7F9522C0084 for ; Tue, 3 Dec 2013 09:51:49 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:reply-to:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=l2krYnMsOOhKCuW9L 4buXgbliXxFUcGN1tOK/tcA8LAPDFPl3IoeIITefixMXAcuZ4S4/LYbkX/Xb5VqG rNsgZt7VmBdHIrnQ55UtqFm//3e3TYFHsx75awweDO3gsJsVzusIz3ASt1XkBM0H C3IwCxcqbSU+Hjs9fppNfIFoL0= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:reply-to:references:mime-version :content-type:in-reply-to; s=default; bh=G1dLeN3E4raYJ4m06NTsFig SC7U=; b=LDtAnqpVTzKKS5g+fjEt+dSdhBOMz3Xp1BgMtAJES1qjJ0O/brLlX9z C3W5Fu62OAcCteih5WcMme0OeogxPJo/+ulPxWUn50FbiAx5gDnBDIgTLBmoigYS jRqMWtdEsH/e9/YMgoV/Ce66ULJxlZyN/JnSI855mCCjp3qoNgqY= Received: (qmail 30844 invoked by alias); 2 Dec 2013 22:51:39 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 30834 invoked by uid 89); 2 Dec 2013 22:51:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL, BAYES_50, RDNS_NONE, SPF_HELO_PASS, SPF_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 02 Dec 2013 22:51:37 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rB2MpSWo007628 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 2 Dec 2013 17:51:28 -0500 Received: from tucnak.zalov.cz (vpn1-7-223.ams2.redhat.com [10.36.7.223]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id rB2MpQYf006259 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 2 Dec 2013 17:51:28 -0500 Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.14.7/8.14.7) with ESMTP id rB2MpP3T016474; Mon, 2 Dec 2013 23:51:26 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.14.7/8.14.7/Submit) id rB2MpPC3016473; Mon, 2 Dec 2013 23:51:25 +0100 Date: Mon, 2 Dec 2013 23:51:25 +0100 From: Jakub Jelinek To: Eric Botcazou Cc: gcc-patches@gcc.gnu.org, Steven Bosscher , Richard Biener Subject: [PATCH] Fix up cmove expansion (PR target/58864, take 2) Message-ID: <20131202225124.GS892@tucnak.redhat.com> Reply-To: Jakub Jelinek References: <20131129215630.GG892@tucnak.redhat.com> <1590476.2x66Irb6tH@polaris> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1590476.2x66Irb6tH@polaris> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes 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 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 --- 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; + } +}