[PATCH][AArch64] Simplify frame pointer logic
Simplify frame pointer logic based on review comments here (https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01727.html). This patch incrementally adds to these frame pointer cleanup patches: https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00377.html https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00381.html Add aarch64_needs_frame_chain to decide when to emit the frame chain using clearer logic. Introduce aarch64_use_frame_pointer which contains the value of -fno-omit-frame-pointer (flag_omit_frame_pointer is set to a magic value so that the mid-end won't force the frame pointer in all cases, and leaf frame pointer emission can't be supported). OK for commit? 2017-10-25 Wilco Dijkstra * config/aarch64/aarch64.c (aarch64_use_frame_pointer): Add new boolean. (aarch64_needs_frame_chain): New function. (aarch64_parse_override_string): Set aarch64_parse_override_string. -- diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 0c0edc100ee4197d027e2d80da56a12b9129fca4..9abde0d66d52ac4fd17d1dc2193a6bbb5f9c65fc 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -163,6 +163,9 @@ unsigned long aarch64_tune_flags = 0; /* Global flag for PC relative loads. */ bool aarch64_pcrelative_literal_loads; +/* Global flag for whether frame pointer is enabled. */ +bool aarch64_use_frame_pointer; + /* Support for command line parsing of boolean flags in the tuning structures. */ struct aarch64_flag_desc @@ -2871,6 +2874,25 @@ aarch64_output_probe_stack_range (rtx reg1, rtx reg2) return ""; } +/* Determine whether a frame chain needs to be generated. */ +static bool +aarch64_needs_frame_chain (void) +{ + /* Force a frame chain for EH returns so the return address is at FP+8. */ + if (frame_pointer_needed || crtl->calls_eh_return) +return true; + + /* A leaf function cannot have calls or write LR. */ + bool is_leaf = crtl->is_leaf && !df_regs_ever_live_p (LR_REGNUM); + + /* Don't use a frame chain in leaf functions if leaf frame pointers + are disabled. */ + if (flag_omit_leaf_frame_pointer && is_leaf) +return false; + + return aarch64_use_frame_pointer; +} + /* Mark the registers that need to be saved by the callee and calculate the size of the callee-saved registers area and frame record (both FP and LR may be omitted). */ @@ -2883,17 +2905,7 @@ aarch64_layout_frame (void) if (reload_completed && cfun->machine->frame.laid_out) return; - /* Force a frame chain for EH returns so the return address is at FP+8. */ - cfun->machine->frame.emit_frame_chain -= frame_pointer_needed || crtl->calls_eh_return; - - /* Emit a frame chain if the frame pointer is enabled. - If -momit-leaf-frame-pointer is used, do not use a frame chain - in leaf functions which do not use LR. */ - if (flag_omit_frame_pointer == 2 - && !(flag_omit_leaf_frame_pointer && crtl->is_leaf - && !df_regs_ever_live_p (LR_REGNUM))) -cfun->machine->frame.emit_frame_chain = true; + cfun->machine->frame.emit_frame_chain = aarch64_needs_frame_chain (); #define SLOT_NOT_REQUIRED (-2) #define SLOT_REQUIRED (-1) @@ -9152,12 +9164,12 @@ aarch64_override_options_after_change_1 (struct gcc_options *opts) /* PR 70044: We have to be careful about being called multiple times for the same function. This means all changes should be repeatable. */ - /* If the frame pointer is enabled, set it to a special value that behaves - similar to frame pointer omission. If we don't do this all leaf functions - will get a frame pointer even if flag_omit_leaf_frame_pointer is set. - If flag_omit_frame_pointer has this special value, we must force the - frame pointer if not in a leaf function. We also need to force it in a - leaf function if flag_omit_frame_pointer is not set or if LR is used. */ + /* Set aarch64_use_frame_pointer based on -fno-omit-frame-pointer. + Disable the frame pointer flag so the mid-end will not use a frame + pointer in leaf functions in order to support -fomit-leaf-frame-pointer. + Set x_flag_omit_frame_pointer to the special value 2 to differentiate + between -fomit-frame-pointer (1) and -fno-omit-frame-pointer (2). */ + aarch64_use_frame_pointer = opts->x_flag_omit_frame_pointer != 1; if (opts->x_flag_omit_frame_pointer == 0) opts->x_flag_omit_frame_pointer = 2;
Re: [PATCH][AArch64] Simplify frame pointer logic
ping From: Wilco Dijkstra Sent: 25 October 2017 16:29 To: GCC Patches Cc: nd Subject: [PATCH][AArch64] Simplify frame pointer logic Simplify frame pointer logic based on review comments here (https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01727.html). This patch incrementally adds to these frame pointer cleanup patches: https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00377.html https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00381.html Add aarch64_needs_frame_chain to decide when to emit the frame chain using clearer logic. Introduce aarch64_use_frame_pointer which contains the value of -fno-omit-frame-pointer (flag_omit_frame_pointer is set to a magic value so that the mid-end won't force the frame pointer in all cases, and leaf frame pointer emission can't be supported). OK for commit? 2017-10-25 Wilco Dijkstra * config/aarch64/aarch64.c (aarch64_use_frame_pointer): Add new boolean. (aarch64_needs_frame_chain): New function. (aarch64_parse_override_string): Set aarch64_parse_override_string. -- diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 0c0edc100ee4197d027e2d80da56a12b9129fca4..9abde0d66d52ac4fd17d1dc2193a6bbb5f9c65fc 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -163,6 +163,9 @@ unsigned long aarch64_tune_flags = 0; /* Global flag for PC relative loads. */ bool aarch64_pcrelative_literal_loads; +/* Global flag for whether frame pointer is enabled. */ +bool aarch64_use_frame_pointer; + /* Support for command line parsing of boolean flags in the tuning structures. */ struct aarch64_flag_desc @@ -2871,6 +2874,25 @@ aarch64_output_probe_stack_range (rtx reg1, rtx reg2) return ""; } +/* Determine whether a frame chain needs to be generated. */ +static bool +aarch64_needs_frame_chain (void) +{ + /* Force a frame chain for EH returns so the return address is at FP+8. */ + if (frame_pointer_needed || crtl->calls_eh_return) + return true; + + /* A leaf function cannot have calls or write LR. */ + bool is_leaf = crtl->is_leaf && !df_regs_ever_live_p (LR_REGNUM); + + /* Don't use a frame chain in leaf functions if leaf frame pointers + are disabled. */ + if (flag_omit_leaf_frame_pointer && is_leaf) + return false; + + return aarch64_use_frame_pointer; +} + /* Mark the registers that need to be saved by the callee and calculate the size of the callee-saved registers area and frame record (both FP and LR may be omitted). */ @@ -2883,17 +2905,7 @@ aarch64_layout_frame (void) if (reload_completed && cfun->machine->frame.laid_out) return; - /* Force a frame chain for EH returns so the return address is at FP+8. */ - cfun->machine->frame.emit_frame_chain - = frame_pointer_needed || crtl->calls_eh_return; - - /* Emit a frame chain if the frame pointer is enabled. - If -momit-leaf-frame-pointer is used, do not use a frame chain - in leaf functions which do not use LR. */ - if (flag_omit_frame_pointer == 2 - && !(flag_omit_leaf_frame_pointer && crtl->is_leaf - && !df_regs_ever_live_p (LR_REGNUM))) - cfun->machine->frame.emit_frame_chain = true; + cfun->machine->frame.emit_frame_chain = aarch64_needs_frame_chain (); #define SLOT_NOT_REQUIRED (-2) #define SLOT_REQUIRED (-1) @@ -9152,12 +9164,12 @@ aarch64_override_options_after_change_1 (struct gcc_options *opts) /* PR 70044: We have to be careful about being called multiple times for the same function. This means all changes should be repeatable. */ - /* If the frame pointer is enabled, set it to a special value that behaves - similar to frame pointer omission. If we don't do this all leaf functions - will get a frame pointer even if flag_omit_leaf_frame_pointer is set. - If flag_omit_frame_pointer has this special value, we must force the - frame pointer if not in a leaf function. We also need to force it in a - leaf function if flag_omit_frame_pointer is not set or if LR is used. */ + /* Set aarch64_use_frame_pointer based on -fno-omit-frame-pointer. + Disable the frame pointer flag so the mid-end will not use a frame + pointer in leaf functions in order to support -fomit-leaf-frame-pointer. + Set x_flag_omit_frame_pointer to the special value 2 to differentiate + between -fomit-frame-pointer (1) and -fno-omit-frame-pointer (2). */ + aarch64_use_frame_pointer = opts->x_flag_omit_frame_pointer != 1; if (opts->x_flag_omit_frame_pointer == 0) opts->x_flag_omit_frame_pointer = 2;
Re: [PATCH][AArch64] Simplify frame pointer logic
On Tue, May 15, 2018 at 08:11:21AM -0500, Wilco Dijkstra wrote: > > ping > > > From: Wilco Dijkstra > Sent: 25 October 2017 16:29 > To: GCC Patches > Cc: nd > Subject: [PATCH][AArch64] Simplify frame pointer logic > > > Simplify frame pointer logic based on review comments here > (https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01727.html). > > This patch incrementally adds to these frame pointer cleanup patches: > https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00377.html > https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00381.html > > Add aarch64_needs_frame_chain to decide when to emit the frame > chain using clearer logic. Introduce aarch64_use_frame_pointer > which contains the value of -fno-omit-frame-pointer > (flag_omit_frame_pointer is set to a magic value so that the mid-end > won't force the frame pointer in all cases, and leaf frame pointer > emission can't be supported). > > OK for commit? > > +/* Determine whether a frame chain needs to be generated. */ > +static bool > +aarch64_needs_frame_chain (void) > +{ > + /* Force a frame chain for EH returns so the return address is at FP+8. */ > + if (frame_pointer_needed || crtl->calls_eh_return) > + return true; To match the original logic, I think this needs to not return true, but set some temporary to true which may be overwritten by... > + > + /* A leaf function cannot have calls or write LR. */ > + bool is_leaf = crtl->is_leaf && !df_regs_ever_live_p (LR_REGNUM); > + > + /* Don't use a frame chain in leaf functions if leaf frame pointers > + are disabled. */ > + if (flag_omit_leaf_frame_pointer && is_leaf) > + return false; This. > + > + return aarch64_use_frame_pointer; > +} > + I say that because here > - /* Force a frame chain for EH returns so the return address is at FP+8. */ > - cfun->machine->frame.emit_frame_chain > - = frame_pointer_needed || crtl->calls_eh_return; > - We fall through to the next check. > - /* Emit a frame chain if the frame pointer is enabled. > - If -momit-leaf-frame-pointer is used, do not use a frame chain > - in leaf functions which do not use LR. */ > - if (flag_omit_frame_pointer == 2 > - && !(flag_omit_leaf_frame_pointer && crtl->is_leaf > - && !df_regs_ever_live_p (LR_REGNUM))) > - cfun->machine->frame.emit_frame_chain = true; > + cfun->machine->frame.emit_frame_chain = aarch64_needs_frame_chain (); That may well have been a long-standing bug, but I wanted to query it as you don't mention any bug fixes in the patch cover letter. Thanks, James
Re: [PATCH][AArch64] Simplify frame pointer logic
James Greenhalgh wrote: > +/* Determine whether a frame chain needs to be generated. */ > +static bool > +aarch64_needs_frame_chain (void) > +{ > + /* Force a frame chain for EH returns so the return address is at FP+8. */ > + if (frame_pointer_needed || crtl->calls_eh_return) > + return true; > To match the original logic, I think this needs to not return true, but set > some temporary to true which may be overwritten by... It's only overwritten if false, once true it cannot ever become false. > + > + /* A leaf function cannot have calls or write LR. */ > + bool is_leaf = crtl->is_leaf && !df_regs_ever_live_p (LR_REGNUM); > + > + /* Don't use a frame chain in leaf functions if leaf frame pointers > + are disabled. */ > + if (flag_omit_leaf_frame_pointer && is_leaf) > + return false; > This. No, a leaf function with alloca or EH return still must use a frame pointer. > + > + return aarch64_use_frame_pointer; > +} > + > I say that because here > - /* Force a frame chain for EH returns so the return address is at FP+8. */ > - cfun->machine->frame.emit_frame_chain > - = frame_pointer_needed || crtl->calls_eh_return; > - > We fall through to the next check. > - /* Emit a frame chain if the frame pointer is enabled. > - If -momit-leaf-frame-pointer is used, do not use a frame chain > - in leaf functions which do not use LR. */ > - if (flag_omit_frame_pointer == 2 > - && !(flag_omit_leaf_frame_pointer && crtl->is_leaf > - && !df_regs_ever_live_p (LR_REGNUM))) > - cfun->machine->frame.emit_frame_chain = true; > + cfun->machine->frame.emit_frame_chain = aarch64_needs_frame_chain (); > That may well have been a long-standing bug, but I wanted to query it > as you don't mention any bug fixes in the patch cover letter. There is no bug here, the code is non-trivial but it does exactly what it says - forcing the frame chain on when required in a non-leaf or a leaf if leaf frame pointer omission is disabled. The point of the new code is to make it a bit simpler to read. Wilco
Re: [PATCH][AArch64] Simplify frame pointer logic
On Tue, May 22, 2018 at 10:37:30AM -0500, Wilco Dijkstra wrote: > James Greenhalgh wrote: > > > +/* Determine whether a frame chain needs to be generated. */ > > +static bool > > +aarch64_needs_frame_chain (void) > > +{ > > + /* Force a frame chain for EH returns so the return address is at FP+8. > > */ > > + if (frame_pointer_needed || crtl->calls_eh_return) > > + return true; > > > To match the original logic, I think this needs to not return true, but set > > some temporary to true which may be overwritten by... > > It's only overwritten if false, once true it cannot ever become false. Doh, of course. OK for trunk. Thanks, James