[PATCH][AArch64] Simplify frame pointer logic

[PATCH][AArch64] Simplify frame pointer logic

[Wilco Dijkstra]

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  <wdijkstr@arm.com>

	* 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

[Wilco Dijkstra]

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  <wdijkstr@arm.com>

        * 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

[James Greenhalgh]

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

[Wilco Dijkstra]

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

[James Greenhalgh]

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