| Message ID | DB6PR0801MB205392DA3F942C111AD4572783B60@DB6PR0801MB2053.eurprd08.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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
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
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
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. --