diff mbox

i386: Don't use frame pointer without stack access

Message ID CAMe9rOqYfswAq_3h=vADptn1v6NcpyvALLCxBLOMDJKdZR97=w@mail.gmail.com
State New
Headers show

Commit Message

H.J. Lu Aug. 9, 2017, 12:26 p.m. UTC 
On Wed, Aug 9, 2017 at 4:59 AM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Wed, 9 Aug 2017, H.J. Lu wrote:
>
>> > OK, but then both -f[no-]omit-frame-pointer do not have clearly defined semantics and thus shouldn't be exposed to the user?
>>
>> -f[no-]omit-frame-pointer apply to cases where a new stack frame
>> is needed.  -fno-omit-frame-pointer allows you to unwind each
>> stack frame, not necessarily each function, via frame pointer.
>>  -fno-omit-frame-pointer may not create a new stack frame for
>> each function, similar to LTO or function inlining.  But you can still
>> unwind via frame pointer.  We never guarantee that we will create
>> a new stack frame for each function.  Some functions are inlined
>> completely.  Some just use the caller's stack frame.
>
> Maybe something along these lines should be added to the docu of
> fno-omit-frame-pointer then.  At least "Note that -fno-omit-frame-pointer
> doesn't force a frame for all functions if it isn't otherwise needed, and
> hence doesn't guarantee a frame pointer for all functions." .
>

Like this?

Note that @option{-fno-omit-frame-pointer} doesn't force a new stack
frame for all functions if it isn't otherwise needed, and hence doesn't
guarantee a new frame pointer for all functions.

Here is the updated patch with a note for -fno-omit-frame-pointer.

OK for trunk?

Comments

Andi Kleen Aug. 9, 2017, 3:04 p.m. UTC | #1 
"H.J. Lu" <hjl.tools@gmail.com> writes:
>
> Like this?
>
> Note that @option{-fno-omit-frame-pointer} doesn't force a new stack
> frame for all functions if it isn't otherwise needed, and hence doesn't
> guarantee a new frame pointer for all functions.
>
> Here is the updated patch with a note for -fno-omit-frame-pointer.
>
> OK for trunk?

I suspect there will be still some unwinder or code patching
setups which rely on frame pointer everywhere become broken. But
doing the optimization for -fno-omit-frame-pointer by default
seems reasonable.

I would add a new option
-fforce-frame-pointer
that gives the old -fno-omit-frame-pointer back, so that
users relying on frame pointers everywhere have a workaround.

-Andi
Arjan van de Ven Aug. 9, 2017, 3:05 p.m. UTC | #2 
On 8/9/2017 8:04 AM, Andi Kleen wrote:
> I would add a new option
> -fforce-frame-pointer
> that gives the old -fno-omit-frame-pointer back, so that
> users relying on frame pointers everywhere have a workaround.

that function should also fix the current situation where the framepointer is not useful
because all actual code was moved to be outside the frame pointer code
(e.g.  push/mov/pop followed by the actual function code)
H.J. Lu Aug. 9, 2017, 3:13 p.m. UTC | #3 
On Wed, Aug 9, 2017 at 8:05 AM, Arjan van de Ven <arjan@linux.intel.com> wrote:
> On 8/9/2017 8:04 AM, Andi Kleen wrote:
>>
>> I would add a new option
>> -fforce-frame-pointer
>> that gives the old -fno-omit-frame-pointer back, so that
>> users relying on frame pointers everywhere have a workaround.

This must be much more specific.  How does it impact:

1. LTO
2. Function inlining.
3. Partial function inlining.
4. Shrink-wrapping.

Any of them can impact function stack frame.

>
> that function should also fix the current situation where the framepointer
> is not useful
> because all actual code was moved to be outside the frame pointer code
> (e.g.  push/mov/pop followed by the actual function code)
>
Andi Kleen Aug. 9, 2017, 3:26 p.m. UTC | #4 
> This must be much more specific.  How does it impact:
> 
> 1. LTO
> 2. Function inlining.
> 3. Partial function inlining.
> 4. Shrink-wrapping.
> 
> Any of them can impact function stack frame.

It doesn't. It's just to get back to the previous state.

Also these others already have explicit options to disable them.


-Andi
H.J. Lu Aug. 9, 2017, 5:28 p.m. UTC | #5 
On Wed, Aug 9, 2017 at 8:26 AM, Andi Kleen <andi@firstfloor.org> wrote:
>> This must be much more specific.  How does it impact:
>>
>> 1. LTO
>> 2. Function inlining.
>> 3. Partial function inlining.
>> 4. Shrink-wrapping.
>>
>> Any of them can impact function stack frame.
>
> It doesn't. It's just to get back to the previous state.
>
> Also these others already have explicit options to disable them.
>

How about

item -fkeep-frame-pointer
@opindex fkeep-frame-pointer
Keep the frame pointer in a register for functions.  This option always
forces a new stack frame for all functions regardless of whether
@option{-fomit-frame-pointer} is enabled or not.  Disabled by default.
diff mbox

Patch

From 4f6197699ab0904671919187c71d4a54594c1431 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sun, 6 Aug 2017 06:24:36 -0700
Subject: [PATCH] i386: Don't use frame pointer without stack access

When there is no stack access, there is no need to use frame pointer
even if -fno-omit-frame-pointer is used and caller's frame pointer is
unchanged.

gcc/

	PR target/81736
	* config/i386/i386.c (ix86_finalize_stack_realign_flags): Renamed
	to ...
	(ix86_finalize_stack_frame_flags): This.  Also clear
	frame_pointer_needed if -fno-omit-frame-pointer is used without
	stack access.
	(ix86_expand_prologue): Replace ix86_finalize_stack_realign_flags
	with ix86_finalize_stack_frame_flags.
	(ix86_expand_epilogue): Likewise.
	(ix86_expand_split_stack_prologue): Likewise.
	* doc/invoke.texi: Add a note for -fno-omit-frame-pointer.

gcc/testsuite/

	PR target/81736
	* gcc.target/i386/pr81736-1.c: New test.
	* gcc.target/i386/pr81736-2.c: Likewise.
	* gcc.target/i386/pr81736-3.c: Likewise.
	* gcc.target/i386/pr81736-4.c: Likewise.
	* gcc.target/i386/pr81736-5.c: Likewise.
	* gcc.target/i386/pr81736-6.c: Likewise.
	* gcc.target/i386/pr81736-7.c: Likewise.
---
 gcc/config/i386/i386.c                    | 23 ++++++++++++-----------
 gcc/doc/invoke.texi                       |  4 ++++
 gcc/testsuite/gcc.target/i386/pr81736-1.c | 13 +++++++++++++
 gcc/testsuite/gcc.target/i386/pr81736-2.c | 14 ++++++++++++++
 gcc/testsuite/gcc.target/i386/pr81736-3.c | 11 +++++++++++
 gcc/testsuite/gcc.target/i386/pr81736-4.c | 11 +++++++++++
 gcc/testsuite/gcc.target/i386/pr81736-5.c | 20 ++++++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr81736-6.c | 16 ++++++++++++++++
 gcc/testsuite/gcc.target/i386/pr81736-7.c | 13 +++++++++++++
 9 files changed, 114 insertions(+), 11 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr81736-1.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr81736-2.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr81736-3.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr81736-4.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr81736-5.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr81736-6.c
 create mode 100644 gcc/testsuite/gcc.target/i386/pr81736-7.c

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 3f8519777f7..85aa4d4fbf5 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -14179,10 +14179,11 @@  output_probe_stack_range (rtx reg, rtx end)
   return "";
 }
 
-/* Finalize stack_realign_needed flag, which will guide prologue/epilogue
-   to be generated in correct form.  */
+/* Finalize stack_realign_needed and frame_pointer_needed flags, which
+   will guide prologue/epilogue to be generated in correct form.  */
+
 static void
-ix86_finalize_stack_realign_flags (void)
+ix86_finalize_stack_frame_flags (void)
 {
   /* Check if stack realign is really needed after reload, and
      stores result in cfun */
@@ -14205,13 +14206,13 @@  ix86_finalize_stack_realign_flags (void)
     }
 
   /* If the only reason for frame_pointer_needed is that we conservatively
-     assumed stack realignment might be needed, but in the end nothing that
-     needed the stack alignment had been spilled, clear frame_pointer_needed
-     and say we don't need stack realignment.  */
-  if (stack_realign
+     assumed stack realignment might be needed or -fno-omit-frame-pointer
+     is used, but in the end nothing that needed the stack alignment had
+     been spilled nor stack access, clear frame_pointer_needed and say we
+     don't need stack realignment.  */
+  if ((stack_realign || !flag_omit_frame_pointer)
       && frame_pointer_needed
       && crtl->is_leaf
-      && flag_omit_frame_pointer
       && crtl->sp_is_unchanging
       && !ix86_current_function_calls_tls_descriptor
       && !crtl->accesses_prior_frames
@@ -14402,7 +14403,7 @@  ix86_expand_prologue (void)
   if (ix86_function_naked (current_function_decl))
     return;
 
-  ix86_finalize_stack_realign_flags ();
+  ix86_finalize_stack_frame_flags ();
 
   /* DRAP should not coexist with stack_realign_fp */
   gcc_assert (!(crtl->drap_reg && stack_realign_fp));
@@ -15266,7 +15267,7 @@  ix86_expand_epilogue (int style)
       return;
     }
 
-  ix86_finalize_stack_realign_flags ();
+  ix86_finalize_stack_frame_flags ();
   frame = m->frame;
 
   m->fs.sp_realigned = stack_realign_fp;
@@ -15801,7 +15802,7 @@  ix86_expand_split_stack_prologue (void)
 
   gcc_assert (flag_split_stack && reload_completed);
 
-  ix86_finalize_stack_realign_flags ();
+  ix86_finalize_stack_frame_flags ();
   frame = cfun->machine->frame;
   allocate = frame.stack_pointer_offset - INCOMING_FRAME_SP_OFFSET;
 
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index cc0f5a00a4f..3753d8a992b 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -7366,6 +7366,10 @@  size) for 32-bit GNU/Linux x86 and 32-bit Darwin x86 targets is
 @option{-fomit-frame-pointer}.  You can configure GCC with the
 @option{--enable-frame-pointer} configure option to change the default.
 
+Note that @option{-fno-omit-frame-pointer} doesn't force a new stack
+frame for all functions if it isn't otherwise needed, and hence doesn't
+guarantee a new frame pointer for all functions.
+
 Enabled at levels @option{-O}, @option{-O2}, @option{-O3}, @option{-Os}.
 
 @item -foptimize-sibling-calls
diff --git a/gcc/testsuite/gcc.target/i386/pr81736-1.c b/gcc/testsuite/gcc.target/i386/pr81736-1.c
new file mode 100644
index 00000000000..92c7bc97a0d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr81736-1.c
@@ -0,0 +1,13 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-omit-frame-pointer" } */
+
+extern int i;
+
+int
+foo (void)
+{
+  return i;
+}
+
+/* No need to use a frame pointer.  */
+/* { dg-final { scan-assembler-not "%\[re\]bp" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr81736-2.c b/gcc/testsuite/gcc.target/i386/pr81736-2.c
new file mode 100644
index 00000000000..a3720879937
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr81736-2.c
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-omit-frame-pointer" } */
+
+int
+#ifndef __x86_64__
+__attribute__((regparm(3)))
+#endif
+foo (int i)
+{
+  return i;
+}
+
+/* No need to use a frame pointer.  */
+/* { dg-final { scan-assembler-not "%\[re\]bp" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr81736-3.c b/gcc/testsuite/gcc.target/i386/pr81736-3.c
new file mode 100644
index 00000000000..c3bde7dd933
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr81736-3.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-omit-frame-pointer" } */
+
+void
+foo (void)
+{
+  asm ("# " : : : "ebx");
+}
+
+/* Need to use a frame pointer.  */
+/* { dg-final { scan-assembler "%\[re\]bp" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr81736-4.c b/gcc/testsuite/gcc.target/i386/pr81736-4.c
new file mode 100644
index 00000000000..25f50016a64
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr81736-4.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-omit-frame-pointer" } */
+
+int
+foo (int i1, int i2, int i3, int i4, int i5, int i6, int i7)
+{
+  return i7;
+}
+
+/* Need to use a frame pointer.  */
+/* { dg-final { scan-assembler "%\[re\]bp" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr81736-5.c b/gcc/testsuite/gcc.target/i386/pr81736-5.c
new file mode 100644
index 00000000000..e1602cf25ba
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr81736-5.c
@@ -0,0 +1,20 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-omit-frame-pointer -mavx" } */
+
+typedef int v8si __attribute__ ((vector_size (32)));
+
+void
+#ifndef __x86_64__
+__attribute__((regparm(3)))
+#endif
+foo (v8si *out_start, v8si *out_end, v8si *regions)
+{
+  v8si base = regions[3];
+  *out_start = base;
+  *out_end = base;
+}
+
+/* No need to use a frame pointer.  */
+/* { dg-final { scan-assembler-not "%\[re\]bp" } } */
+/* Verify no dynamic realignment is performed.  */
+/* { dg-final { scan-assembler-not "and\[^\n\r]*sp" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr81736-6.c b/gcc/testsuite/gcc.target/i386/pr81736-6.c
new file mode 100644
index 00000000000..6198574c8cc
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr81736-6.c
@@ -0,0 +1,16 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-omit-frame-pointer" } */
+
+struct foo
+{
+  int head;
+} a;
+
+int
+bar (void)
+{
+  return a.head != 0;
+}
+
+/* No need to use a frame pointer.  */
+/* { dg-final { scan-assembler-not "%\[re\]bp" } } */
diff --git a/gcc/testsuite/gcc.target/i386/pr81736-7.c b/gcc/testsuite/gcc.target/i386/pr81736-7.c
new file mode 100644
index 00000000000..f947886e642
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr81736-7.c
@@ -0,0 +1,13 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fno-omit-frame-pointer" } */
+
+extern int foo (void);
+
+int
+bar (void)
+{
+  return foo ();
+}
+
+/* No need to use a frame pointer.  */
+/* { dg-final { scan-assembler-not "%\[re\]bp" } } */
-- 
2.13.4