[PATCH][AArch64] Remove aarch64_frame_pointer_required

[PATCH][AArch64] Remove aarch64_frame_pointer_required

[Wilco Dijkstra]

To implement -fomit-leaf-frame-pointer, there are 2 places where we need
to check whether we have to use a frame chain (since register allocation
may allocate LR in a leaf function that omits the frame pointer, but if
LR is spilled we must emit a frame chain).  To simplify this do not force
frame_pointer_needed via aarch64_frame_pointer_required, but enable the
frame chain in aarch64_layout_frame.  Now aarch64_frame_pointer_required
can be removed and aarch64_can_eliminate is simplified.

OK for commit?

ChangeLog:
2017-08-03  Wilco Dijkstra  <wdijkstr@arm.com>

    gcc/
	* config/aarch64/aarch64.c (aarch64_frame_pointer_required)
	Remove.
	(aarch64_layout_frame): Initialise emit_frame_chain.
	(aarch64_can_eliminate): Remove omit leaf frame pointer code.
	(TARGET_FRAME_POINTER_REQUIRED): Remove define.
--

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index aa71a410106164bb8da808a4b513771d713bb0f0..9bbc9864fd47a4404a80ea0cd5608202e8d0726a 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -2836,21 +2836,6 @@ aarch64_output_probe_stack_range (rtx reg1, rtx reg2)
   return "";
 }
 
-static bool
-aarch64_frame_pointer_required (void)
-{
-  /* Use the frame pointer if enabled and it is not a leaf function, unless
-     leaf frame pointer omission is disabled.  If the frame pointer is enabled,
-     force the frame pointer in leaf functions which use LR.  */
-  if (flag_omit_frame_pointer == 2
-      && !(flag_omit_leaf_frame_pointer
-	   && crtl->is_leaf
-	   && !df_regs_ever_live_p (LR_REGNUM)))
-    return true;
-
-  return false;
-}
-
 /* 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).  */
@@ -2867,6 +2852,14 @@ aarch64_layout_frame (void)
   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;
+
 #define SLOT_NOT_REQUIRED (-2)
 #define SLOT_REQUIRED     (-1)
 
@@ -5884,17 +5877,6 @@ aarch64_can_eliminate (const int from, const int to)
 
       return false;
     }
-  else
-    {
-      /* If we decided that we didn't need a leaf frame pointer but then used
-	 LR in the function, then we'll want a frame pointer after all, so
-	 prevent this elimination to ensure a frame pointer is used.  */
-      if (to == STACK_POINTER_REGNUM
-	  && flag_omit_frame_pointer == 2
-	  && flag_omit_leaf_frame_pointer
-	  && df_regs_ever_live_p (LR_REGNUM))
-	return false;
-    }
 
   return true;
 }
@@ -15385,9 +15367,6 @@ aarch64_run_selftests (void)
 #undef TARGET_FUNCTION_VALUE_REGNO_P
 #define TARGET_FUNCTION_VALUE_REGNO_P aarch64_function_value_regno_p
 
-#undef TARGET_FRAME_POINTER_REQUIRED
-#define TARGET_FRAME_POINTER_REQUIRED aarch64_frame_pointer_required
-
 #undef TARGET_GIMPLE_FOLD_BUILTIN
 #define TARGET_GIMPLE_FOLD_BUILTIN aarch64_gimple_fold_builtin
 

Re: [PATCH][AArch64] Remove aarch64_frame_pointer_required

[Wilco Dijkstra]

ping


From: Wilco Dijkstra
Sent: 04 August 2017 13:41
To: GCC Patches; James Greenhalgh
Cc: nd
Subject: [PATCH][AArch64] Remove aarch64_frame_pointer_required
    
To implement -fomit-leaf-frame-pointer, there are 2 places where we need
to check whether we have to use a frame chain (since register allocation
may allocate LR in a leaf function that omits the frame pointer, but if
LR is spilled we must emit a frame chain).  To simplify this do not force
frame_pointer_needed via aarch64_frame_pointer_required, but enable the
frame chain in aarch64_layout_frame.  Now aarch64_frame_pointer_required
can be removed and aarch64_can_eliminate is simplified.

OK for commit?

ChangeLog:
2017-08-03  Wilco Dijkstra  <wdijkstr@arm.com>

    gcc/
        * config/aarch64/aarch64.c (aarch64_frame_pointer_required)
        Remove.
        (aarch64_layout_frame): Initialise emit_frame_chain.
        (aarch64_can_eliminate): Remove omit leaf frame pointer code.
        (TARGET_FRAME_POINTER_REQUIRED): Remove define.
--

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index aa71a410106164bb8da808a4b513771d713bb0f0..9bbc9864fd47a4404a80ea0cd5608202e8d0726a 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -2836,21 +2836,6 @@ aarch64_output_probe_stack_range (rtx reg1, rtx reg2)
   return "";
 }
 
-static bool
-aarch64_frame_pointer_required (void)
-{
-  /* Use the frame pointer if enabled and it is not a leaf function, unless
-     leaf frame pointer omission is disabled.  If the frame pointer is enabled,
-     force the frame pointer in leaf functions which use LR.  */
-  if (flag_omit_frame_pointer == 2
-      && !(flag_omit_leaf_frame_pointer
-          && crtl->is_leaf
-          && !df_regs_ever_live_p (LR_REGNUM)))
-    return true;
-
-  return false;
-}
-
 /* 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).  */
@@ -2867,6 +2852,14 @@ aarch64_layout_frame (void)
   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;
+
 #define SLOT_NOT_REQUIRED (-2)
 #define SLOT_REQUIRED     (-1)
 
@@ -5884,17 +5877,6 @@ aarch64_can_eliminate (const int from, const int to)
 
       return false;
     }
-  else
-    {
-      /* If we decided that we didn't need a leaf frame pointer but then used
-        LR in the function, then we'll want a frame pointer after all, so
-        prevent this elimination to ensure a frame pointer is used.  */
-      if (to == STACK_POINTER_REGNUM
-         && flag_omit_frame_pointer == 2
-         && flag_omit_leaf_frame_pointer
-         && df_regs_ever_live_p (LR_REGNUM))
-       return false;
-    }
 
   return true;
 }
@@ -15385,9 +15367,6 @@ aarch64_run_selftests (void)
 #undef TARGET_FUNCTION_VALUE_REGNO_P
 #define TARGET_FUNCTION_VALUE_REGNO_P aarch64_function_value_regno_p
 
-#undef TARGET_FRAME_POINTER_REQUIRED
-#define TARGET_FRAME_POINTER_REQUIRED aarch64_frame_pointer_required
-
 #undef TARGET_GIMPLE_FOLD_BUILTIN
 #define TARGET_GIMPLE_FOLD_BUILTIN aarch64_gimple_fold_builtin
 

    

Re: [PATCH][AArch64] Remove aarch64_frame_pointer_required

[James Greenhalgh]

On Fri, Aug 04, 2017 at 01:41:22PM +0100, Wilco Dijkstra wrote:
> To implement -fomit-leaf-frame-pointer, there are 2 places where we need
> to check whether we have to use a frame chain (since register allocation
> may allocate LR in a leaf function that omits the frame pointer, but if
> LR is spilled we must emit a frame chain).  To simplify this do not force
> frame_pointer_needed via aarch64_frame_pointer_required, but enable the
> frame chain in aarch64_layout_frame.  Now aarch64_frame_pointer_required
> can be removed and aarch64_can_eliminate is simplified.
> 
> OK for commit?

I've thought about this for a while, I'm still not completely comfortable
with the idea that we "lie" to the mid-end about the frame-pointer, and
patch it up in the frame layout code, but I can see how we're moving from
one lie which adds a ton of complexity, to a different lie which reduces
complexity.

It all still seems tenuous, but I think I understand the reasoning, and
we've got a solid 6 months to figure out what breaks.

OK (assuming this has been tested *recently* against aarch64-none-linux-gnu).

Reviewed-By: James Greenhalgh <james.greenhalgh@arm.com>

Thanks,
James

> 
> ChangeLog:
> 2017-08-03  Wilco Dijkstra  <wdijkstr@arm.com>
> 
>     gcc/
> 	* config/aarch64/aarch64.c (aarch64_frame_pointer_required)
> 	Remove.
> 	(aarch64_layout_frame): Initialise emit_frame_chain.
> 	(aarch64_can_eliminate): Remove omit leaf frame pointer code.
> 	(TARGET_FRAME_POINTER_REQUIRED): Remove define.

Re: [PATCH][AArch64] Remove aarch64_frame_pointer_required

[Richard Sandiford]

James Greenhalgh <james.greenhalgh@arm.com> writes:
> On Fri, Aug 04, 2017 at 01:41:22PM +0100, Wilco Dijkstra wrote:
>> To implement -fomit-leaf-frame-pointer, there are 2 places where we need
>> to check whether we have to use a frame chain (since register allocation
>> may allocate LR in a leaf function that omits the frame pointer, but if
>> LR is spilled we must emit a frame chain).  To simplify this do not force
>> frame_pointer_needed via aarch64_frame_pointer_required, but enable the
>> frame chain in aarch64_layout_frame.  Now aarch64_frame_pointer_required
>> can be removed and aarch64_can_eliminate is simplified.
>> 
>> OK for commit?
>
> I've thought about this for a while, I'm still not completely comfortable
> with the idea that we "lie" to the mid-end about the frame-pointer, and
> patch it up in the frame layout code, but I can see how we're moving from
> one lie which adds a ton of complexity, to a different lie which reduces
> complexity.
>
> It all still seems tenuous, but I think I understand the reasoning, and
> we've got a solid 6 months to figure out what breaks.

So this really is a few months later (sorry), but I'm not sure it's safe.
emit_frame_chain was introduced on the basis that:

    The current frame code combines the separate concepts of a frame chain
    (saving old FP,LR in a record and pointing new FP to it) and a frame
    pointer used to access locals.

But there's the third question of whether the frame pointer is available
for general allocation.  By removing frame_pointer_required, we're saying
that the frame pointer is always available for general use.  This can be
forced with a crude test case like:

  void g (void);

  int
  f (void)
  {
    register int x asm ("x29");
    asm volatile ("mov %0, 1" : "=r" (x));
    g ();
    return 1;
  }

which at -O2 (with implicit -fno-omit-frame-pointer) generates:

f:
        stp     x29, x30, [sp, -16]!
        mov     x29, sp
#APP
// 7 "/tmp/foo.c" 1
        mov x29, 1
// 0 "" 2
#NO_APP 
        bl      g
        mov     w0, 1
        ldp     x29, x30, [sp], 16
        ret

So we do initialise the frame pointer, but it's not reliable for
backtracing within g.  The same thing could happen in more realistic
functions with high register pressure.

(The above test would generate an error if frame_pointer_required
returned true.)

If we wanted to continue to use sp-based rather than fp-based
accesses with -fno-omit-frame-pointer where possible, we'd probably
need to do that in target-independent code rather than hide it in the
backend.

Thanks,
Richard

> OK (assuming this has been tested *recently* against aarch64-none-linux-gnu).
>
> Reviewed-By: James Greenhalgh <james.greenhalgh@arm.com>
>
> Thanks,
> James
>
>> 
>> ChangeLog:
>> 2017-08-03  Wilco Dijkstra  <wdijkstr@arm.com>
>> 
>>     gcc/
>> 	* config/aarch64/aarch64.c (aarch64_frame_pointer_required)
>> 	Remove.
>> 	(aarch64_layout_frame): Initialise emit_frame_chain.
>> 	(aarch64_can_eliminate): Remove omit leaf frame pointer code.
>> 	(TARGET_FRAME_POINTER_REQUIRED): Remove define.

Re: [PATCH][AArch64] Remove aarch64_frame_pointer_required

[Wilco Dijkstra]

Richard Sandiford wrote:
> But there's the third question of whether the frame pointer is available
> for general allocation.  By removing frame_pointer_required, we're saying
> that the frame pointer is always available for general use.  

Unlike on ARM/Thumb-2, the frame pointer is unfortunately never available for
general allocation on AArch64 - so we cannot use it for something actually useful.
A while back there were mid-end patches proposed to allow general allocation
of FP but those weren't accepted.

Wilco

Re: [PATCH][AArch64] Remove aarch64_frame_pointer_required

[Richard Sandiford]

Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes:
> Richard Sandiford wrote:
>> But there's the third question of whether the frame pointer is available
>> for general allocation.  By removing frame_pointer_required, we're saying
>> that the frame pointer is always available for general use.  
>
> Unlike on ARM/Thumb-2, the frame pointer is unfortunately never available for
> general allocation on AArch64 - so we cannot use it for something
> actually useful.
> A while back there were mid-end patches proposed to allow general allocation
> of FP but those weren't accepted.

Ah, missed that, sorry.  So the asm example I gave probably is as far as
the "problem" goes, and the argument can be made that that's just doing
what the user asked for.

Sorry for the noise...

Richard