From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21599 invoked by alias); 1 Sep 2016 13:40:49 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 21319 invoked by uid 89); 1 Sep 2016 13:40:48 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.2 required=5.0 tests=BAYES_60,KAM_LAZY_DOMAIN_SECURITY,KAM_LOTSOFHASH,RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=machine_mode, Pmode, pmode, cselib X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 01 Sep 2016 13:40:38 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 9D325462; Thu, 1 Sep 2016 06:40:35 -0700 (PDT) Received: from [10.2.206.201] (e105545-lin.cambridge.arm.com [10.2.206.201]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0A2403F47F; Thu, 1 Sep 2016 06:40:34 -0700 (PDT) Subject: Re: [PATCH][AArch64 - v2] Simplify eh_return implementation To: Wilco Dijkstra , GCC Patches References: Cc: nd From: Ramana Radhakrishnan Message-ID: Date: Thu, 01 Sep 2016 13:40:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-09/txt/msg00041.txt.bz2 On 10/08/16 17:26, Wilco Dijkstra wrote: > I noticed it would still be a good idea to add an extra barrier in the epilog as the > scheduler doesn't appear to handle aliases of frame accesses properly. > > This patch simplifies the handling of the EH return value. We force the use of the > frame pointer so the return location is always at FP + 8. This means we can emit > a simple volatile access in EH_RETURN_HANDLER_RTX without needing md > patterns, splitters and frame offset calculations. The new implementation also > fixes various bugs in aarch64_final_eh_return_addr, which does not work with > -fomit-frame-pointer, alloca or outgoing arguments. > > Bootstrap OK, GCC Regression OK, OK for trunk? Would it be useful to backport > this to GCC6.x? Can you please file a PR for this and add some testcases ? This sounds like a serious enough problem that needs to be looked at probably going back since the dawn of time. Ramana > > ChangeLog: > 2016-08-10 Wilco Dijkstra > gcc/ > * config/aarch64/aarch64.md (eh_return): Remove pattern and splitter. > * config/aarch64/aarch64.h (AARCH64_EH_STACKADJ_REGNUM): Remove. > (EH_RETURN_HANDLER_RTX): New define. > * config/aarch64/aarch64.c (aarch64_frame_pointer_required): > Force frame pointer in EH return functions. > (aarch64_expand_epilogue): Add barrier for eh_return. > (aarch64_final_eh_return_addr): Remove. > (aarch64_eh_return_handler_rtx): New function. > * config/aarch64/aarch64-protos.h (aarch64_final_eh_return_addr): > Remove. > (aarch64_eh_return_handler_rtx): New prototype. > -- > > diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h > index 3cdd69b8af1089a839e5d45cda94bc70a15cd777..327c0a97f6f687604afef249b79ac22628418070 100644 > --- a/gcc/config/aarch64/aarch64-protos.h > +++ b/gcc/config/aarch64/aarch64-protos.h > @@ -358,7 +358,7 @@ int aarch64_hard_regno_mode_ok (unsigned, machine_mode); > int aarch64_hard_regno_nregs (unsigned, machine_mode); > int aarch64_uxt_size (int, HOST_WIDE_INT); > int aarch64_vec_fpconst_pow_of_2 (rtx); > -rtx aarch64_final_eh_return_addr (void); > +rtx aarch64_eh_return_handler_rtx (void); > rtx aarch64_mask_from_zextract_ops (rtx, rtx); > const char *aarch64_output_move_struct (rtx *operands); > rtx aarch64_return_addr (int, rtx); > diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h > index 003fec87e41db618570663f28cc2387a87e8252a..fa81e4b853dafcccc08842955288861ec7e7acca 100644 > --- a/gcc/config/aarch64/aarch64.h > +++ b/gcc/config/aarch64/aarch64.h > @@ -400,9 +400,9 @@ extern unsigned aarch64_architecture_version; > #define ASM_DECLARE_FUNCTION_NAME(STR, NAME, DECL) \ > aarch64_declare_function_name (STR, NAME, DECL) > > -/* The register that holds the return address in exception handlers. */ > -#define AARCH64_EH_STACKADJ_REGNUM (R0_REGNUM + 4) > -#define EH_RETURN_STACKADJ_RTX gen_rtx_REG (Pmode, AARCH64_EH_STACKADJ_REGNUM) > +/* For EH returns X4 contains the stack adjustment. */ > +#define EH_RETURN_STACKADJ_RTX gen_rtx_REG (Pmode, R4_REGNUM) > +#define EH_RETURN_HANDLER_RTX aarch64_eh_return_handler_rtx () > > /* Don't use __builtin_setjmp until we've defined it. */ > #undef DONT_USE_BUILTIN_SETJMP > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index 5a25fce17785af9f9dc12e0f2a9609af09af0b35..bb8baff1e7a06942c8b8f51c1d6b341673401ef9 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -2739,6 +2739,10 @@ aarch64_frame_pointer_required (void) > && (!crtl->is_leaf || df_regs_ever_live_p (LR_REGNUM))) > return true; > > + /* Force a frame pointer for EH returns so the return address is at FP+8. */ > + if (crtl->calls_eh_return) > + return true; > + > return false; > } > > @@ -3298,7 +3302,8 @@ aarch64_expand_epilogue (bool for_sibcall) > + cfun->machine->frame.saved_varargs_size) != 0; > > /* Emit a barrier to prevent loads from a deallocated stack. */ > - if (final_adjust > crtl->outgoing_args_size || cfun->calls_alloca) > + if (final_adjust > crtl->outgoing_args_size || cfun->calls_alloca > + || crtl->calls_eh_return) > { > emit_insn (gen_stack_tie (stack_pointer_rtx, stack_pointer_rtx)); > need_barrier_p = false; > @@ -3366,52 +3371,15 @@ aarch64_expand_epilogue (bool for_sibcall) > emit_jump_insn (ret_rtx); > } > > -/* Return the place to copy the exception unwinding return address to. > - This will probably be a stack slot, but could (in theory be the > - return register). */ > +/* Implement EH_RETURN_HANDLER_RTX. The return address is stored at FP + 8. > + The access needs to be volatile to prevent it from being removed. */ > rtx > -aarch64_final_eh_return_addr (void) > +aarch64_eh_return_handler_rtx (void) > { > - HOST_WIDE_INT fp_offset; > - > - aarch64_layout_frame (); > - > - fp_offset = cfun->machine->frame.frame_size > - - cfun->machine->frame.hard_fp_offset; > - > - if (cfun->machine->frame.reg_offset[LR_REGNUM] < 0) > - return gen_rtx_REG (DImode, LR_REGNUM); > - > - /* DSE and CSELIB do not detect an alias between sp+k1 and fp+k2. This can > - result in a store to save LR introduced by builtin_eh_return () being > - incorrectly deleted because the alias is not detected. > - So in the calculation of the address to copy the exception unwinding > - return address to, we note 2 cases. > - If FP is needed and the fp_offset is 0, it means that SP = FP and hence > - we return a SP-relative location since all the addresses are SP-relative > - in this case. This prevents the store from being optimized away. > - If the fp_offset is not 0, then the addresses will be FP-relative and > - therefore we return a FP-relative location. */ > - > - if (frame_pointer_needed) > - { > - if (fp_offset) > - return gen_frame_mem (DImode, > - plus_constant (Pmode, hard_frame_pointer_rtx, UNITS_PER_WORD)); > - else > - return gen_frame_mem (DImode, > - plus_constant (Pmode, stack_pointer_rtx, UNITS_PER_WORD)); > - } > - > - /* If FP is not needed, we calculate the location of LR, which would be > - at the top of the saved registers block. */ > - > - return gen_frame_mem (DImode, > - plus_constant (Pmode, > - stack_pointer_rtx, > - fp_offset > - + cfun->machine->frame.saved_regs_size > - - 2 * UNITS_PER_WORD)); > + rtx tmp = gen_frame_mem (Pmode, > + plus_constant (Pmode, hard_frame_pointer_rtx, UNITS_PER_WORD)); > + MEM_VOLATILE_P (tmp) = true; > + return tmp; > } > > /* Output code to add DELTA to the first argument, and then jump > diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md > index 21f5a6aba74d28f04b9391ba917453a4cd7de1af..7d86aa7c0cb2fdb30889badc172d2270eeadb1e5 100644 > --- a/gcc/config/aarch64/aarch64.md > +++ b/gcc/config/aarch64/aarch64.md > @@ -591,25 +591,6 @@ > [(set_attr "type" "branch")] > ) > > -(define_insn "eh_return" > - [(unspec_volatile [(match_operand:DI 0 "register_operand" "r")] > - UNSPECV_EH_RETURN)] > - "" > - "#" > - [(set_attr "type" "branch")] > - > -) > - > -(define_split > - [(unspec_volatile [(match_operand:DI 0 "register_operand" "")] > - UNSPECV_EH_RETURN)] > - "reload_completed" > - [(set (match_dup 1) (match_dup 0))] > - { > - operands[1] = aarch64_final_eh_return_addr (); > - } > -) > - > (define_insn "*cb1" > [(set (pc) (if_then_else (EQL (match_operand:GPI 0 "register_operand" "r") > (const_int 0)) >