diff mbox

PATCH: Turn on -fomit-frame-pointer by default for 32bit Linux/x86

Message ID AANLkTinbviPWO-hDfgL=31x=st_LA2frCPPYXtscEAhX@mail.gmail.com
State New
Headers show

Commit Message

H.J. Lu Aug. 12, 2010, 4:19 p.m. UTC 
On Tue, Aug 10, 2010 at 8:52 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sun, Aug 8, 2010 at 7:56 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> Hello!
>>
>> After recent discussions, I would like to propose a transition to
>> -fomit-frame-pointer for x86_32.
>>
>> The transition should be smooth as much as possible, should have
>> option to revert to old behaviour and still providing path for the
>> improvement. And we have learned something from cld issues, too
>> (cough, cough...).
>>
>> I support the idea to change x86_32 defaults w.r.t. frame pointer (and
>> unwind tables) to the same defaults as x86_64 has.
>>
>> The patch should also introduce --enable-frame-pointer configure
>> option (off by default) that would revert back to old x86_32
>> behaviour. So, if there are codes that depend on FP, their users (or
>> distributions) should either (re-)configure the compiler with
>> --enable-frame-pointer or they should use older compiler - 4.5.x will
>> still be supported for many years. OTOH, it looks that users don't
>> care that much whether backtraces on x86_64 are totally accurate, so
>> IMO the sky won't fall down if x86_32 misses some backtraces in the
>> same way. And as I have learned from the discussion, the problem is
>> fixable with some effort on the user's side, thus fixing both targets
>> in one shot.
>>
>> Of course, this change and the option to revert to the previous
>> behaviour should be announced and documented in GCC release notes for
>> 4.6.0.
>>
>> IMO, we have to bite the bullet from time to time in order to improve
>> the generated code. We should not claim that gcc is
>> "no-code-left-behind compiler" - from my experience, introducing new
>> compiler always means that some parts of the code have to be fixed (as
>> in case of the change to -fno-strict-aliasing).
>>
>> Uros.
>>
>
> I tested this patch on Linux/ia32 and Linux/x86-64. There are no regressions.
>
> I don't have good wording for document:
>
> --
> For 32-bit x86 targets, it is not enabled at @option{-Os} by default.
> This option also can be disabled by default on 32-bit x86 targets by
> configuring GCC with the @option{--enable-frame-pointer} configure
> option.
> --
>
> isn't very accurate.  Any suggestions?
>
> Thanks.
>
>
> --
> H.J.
> ---
> 2010-08-09  H.J. Lu  <hongjiu.lu@intel.com>
>
>        * config.gcc: Handle --enable-frame-pointer.
>
>        * configure.ac: Add --enable-frame-pointer.
>        * configure: Regenerated.
>
>        * config/i386/i386.c (override_options): If not optimize for
>        size, use -fomit-frame-pointer and -fasynchronous-unwind-tables
>        by default for 32-bit code unless configured with
>        --enable-frame-pointer.
>

Here is the updated patch.  Any comments?

Thanks.

Comments

Richard Henderson Aug. 12, 2010, 4:28 p.m. UTC | #1 
On 08/12/2010 09:19 AM, H.J. Lu wrote:
>>        * config.gcc: Handle --enable-frame-pointer.
>>
>>        * configure.ac: Add --enable-frame-pointer.
>>        * configure: Regenerated.
>>
>>        * config/i386/i386.c (override_options): If not optimize for
>>        size, use -fomit-frame-pointer and -fasynchronous-unwind-tables
>>        by default for 32-bit code unless configured with
>>        --enable-frame-pointer.
>>
> 
> Here is the updated patch.  Any comments?

This patch is ok by me.


r~
Uros Bizjak Aug. 12, 2010, 4:30 p.m. UTC | #2 
On Thu, Aug 12, 2010 at 6:19 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

> 2010-08-12  H.J. Lu  <hongjiu.lu@intel.com>
>            Uros Bizjak  <ubizjak@gmail.com>
>
>        * config.gcc: Handle --enable-frame-pointer.
>
>        * configure.ac: Add --enable-frame-pointer.
>        * configure: Regenerated.
>
>        * config/i386/i386.c (USE_IX86_FRAME_POINTER): Default to 0.
>        (override_options): Enable -fomit-frame-pointer for 32bit code
>        if compiling for TARGET_MACHO and not optimizing for size
>        unless configured with --enable-frame-pointer.  Enable
>        -fasynchronous-unwind-tables unless configured with
>        --enable-frame-pointer.  Enable -maccumulate-outgoing-args
>        by default unless configured with --enable-frame-pointer.
>

Please change the ChangeLog text to something like:

If not configured with --enable-frame-pointer, enable
-fomit-frame-pointer (but not for TARGET_MACHO or when optimizing for
size), enable -fasynchronous-unwind-tables and
-maccumulate-outgoing-args by default.

Thanks,
Uros.
H.J. Lu Aug. 12, 2010, 4:43 p.m. UTC | #3 
On Thu, Aug 12, 2010 at 9:30 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Thu, Aug 12, 2010 at 6:19 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>
>> 2010-08-12  H.J. Lu  <hongjiu.lu@intel.com>
>>            Uros Bizjak  <ubizjak@gmail.com>
>>
>>        * config.gcc: Handle --enable-frame-pointer.
>>
>>        * configure.ac: Add --enable-frame-pointer.
>>        * configure: Regenerated.
>>
>>        * config/i386/i386.c (USE_IX86_FRAME_POINTER): Default to 0.
>>        (override_options): Enable -fomit-frame-pointer for 32bit code
>>        if compiling for TARGET_MACHO and not optimizing for size
>>        unless configured with --enable-frame-pointer.  Enable
>>        -fasynchronous-unwind-tables unless configured with
>>        --enable-frame-pointer.  Enable -maccumulate-outgoing-args
>>        by default unless configured with --enable-frame-pointer.
>>
>
> Please change the ChangeLog text to something like:
>
> If not configured with --enable-frame-pointer, enable
> -fomit-frame-pointer (but not for TARGET_MACHO or when optimizing for
> size), enable -fasynchronous-unwind-tables and
> -maccumulate-outgoing-args by default.
>

I checked it in with updated ChangeLog.

How should we document it? We currently have

--
@item -fomit-frame-pointer
@opindex fomit-frame-pointer
Don't keep the frame pointer in a register for functions that
don't need one.  This avoids the instructions to save, set up and
restore frame pointers; it also makes an extra register available
in many functions.  @strong{It also makes debugging impossible on
some machines.}

On some machines, such as the VAX, this flag has no effect, because
the standard calling sequence automatically handles the frame pointer
and nothing is saved by pretending it doesn't exist.  The
machine-description macro @code{FRAME_POINTER_REQUIRED} controls
whether a target machine supports this flag.  @xref{Registers,,Register
Usage, gccint, GNU Compiler Collection (GCC) Internals}.

Enabled at levels @option{-O}, @option{-O2}, @option{-O3}, @option{-Os}.
--

It was never correct for x86 and is wrong today.
Ryan Hill Jan. 30, 2013, 10:02 p.m. UTC | #4 
This change seems to have broken bootstrap with -march=pentium{2,3,-m} on
the 4.6 branch.

http://gcc.gnu.org/PR53728
diff mbox

Patch

diff --git a/gcc/config.gcc b/gcc/config.gcc
index 9170fc8..62dd9f6 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -406,6 +406,9 @@  i[34567]86-*-*)
 	if test "x$enable_cld" = xyes; then
 		tm_defines="${tm_defines} USE_IX86_CLD=1"
 	fi
+	if test "x$enable_frame_pointer" = xyes; then
+		tm_defines="${tm_defines} USE_IX86_FRAME_POINTER=1"
+	fi
 	tm_file="vxworks-dummy.h ${tm_file}"
 	;;
 x86_64-*-*)
@@ -413,6 +416,9 @@  x86_64-*-*)
 	if test "x$enable_cld" = xyes; then
 		tm_defines="${tm_defines} USE_IX86_CLD=1"
 	fi
+	if test "x$enable_frame_pointer" = xyes; then
+		tm_defines="${tm_defines} USE_IX86_FRAME_POINTER=1"
+	fi
 	tm_file="vxworks-dummy.h ${tm_file}"
 	;;
 esac
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index c3863ac..a87175c 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -2979,32 +2979,6 @@  override_options (bool main_args_p)
   if (TARGET_MACHO && TARGET_64BIT)
     flag_pic = 2;
 
-  /* Set the default values for switches whose default depends on TARGET_64BIT
-     in case they weren't overwritten by command line options.  */
-  if (TARGET_64BIT)
-    {
-      if (flag_zee == 2)
-        flag_zee = 1;
-      /* Mach-O doesn't support omitting the frame pointer for now.  */
-      if (flag_omit_frame_pointer == 2)
-	flag_omit_frame_pointer = (TARGET_MACHO ? 0 : 1);
-      if (flag_asynchronous_unwind_tables == 2)
-	flag_asynchronous_unwind_tables = 1;
-      if (flag_pcc_struct_return == 2)
-	flag_pcc_struct_return = 0;
-    }
-  else
-    {
-      if (flag_zee == 2)
-        flag_zee = 0;
-      if (flag_omit_frame_pointer == 2)
-	flag_omit_frame_pointer = 0;
-      if (flag_asynchronous_unwind_tables == 2)
-	flag_asynchronous_unwind_tables = 0;
-      if (flag_pcc_struct_return == 2)
-	flag_pcc_struct_return = DEFAULT_PCC_STRUCT_RETURN;
-    }
-
   /* Need to check -mtune=generic first.  */
   if (ix86_tune_string)
     {
@@ -3292,6 +3266,38 @@  override_options (bool main_args_p)
   for (i = 0; i < X86_TUNE_LAST; ++i)
     ix86_tune_features[i] = !!(initial_ix86_tune_features[i] & ix86_tune_mask);
 
+#ifndef USE_IX86_FRAME_POINTER
+#define USE_IX86_FRAME_POINTER 0
+#endif
+
+  /* Set the default values for switches whose default depends on TARGET_64BIT
+     in case they weren't overwritten by command line options.  */
+  if (TARGET_64BIT)
+    {
+      if (flag_zee == 2)
+        flag_zee = 1;
+      /* Mach-O doesn't support omitting the frame pointer for now.  */
+      if (flag_omit_frame_pointer == 2)
+	flag_omit_frame_pointer = (TARGET_MACHO ? 0 : 1);
+      if (flag_asynchronous_unwind_tables == 2)
+	flag_asynchronous_unwind_tables = 1;
+      if (flag_pcc_struct_return == 2)
+	flag_pcc_struct_return = 0;
+    }
+  else
+    {
+      if (flag_zee == 2)
+        flag_zee = 0;
+      /* Mach-O doesn't support omitting the frame pointer for now.  */
+      if (flag_omit_frame_pointer == 2)
+	flag_omit_frame_pointer =
+	  (TARGET_MACHO ? 0 : !(USE_IX86_FRAME_POINTER || optimize_size));
+      if (flag_asynchronous_unwind_tables == 2)
+	flag_asynchronous_unwind_tables = !USE_IX86_FRAME_POINTER;
+      if (flag_pcc_struct_return == 2)
+	flag_pcc_struct_return = DEFAULT_PCC_STRUCT_RETURN;
+    }
+
   if (optimize_size)
     ix86_cost = &ix86_size_cost;
   else
@@ -3574,7 +3580,8 @@  override_options (bool main_args_p)
 	       prefix, suffix, sw);
     }
 
-  if ((x86_accumulate_outgoing_args & ix86_tune_mask)
+  if ((!USE_IX86_FRAME_POINTER
+       || (x86_accumulate_outgoing_args & ix86_tune_mask))
       && !(target_flags_explicit & MASK_ACCUMULATE_OUTGOING_ARGS)
       && !optimize_size)
     target_flags |= MASK_ACCUMULATE_OUTGOING_ARGS;
diff --git a/gcc/configure b/gcc/configure
index aa61cd6..ad548fc 100755
--- a/gcc/configure
+++ b/gcc/configure
@@ -898,6 +898,7 @@  with_system_libunwind
 enable_secureplt
 enable_leading_mingw64_underscores
 enable_cld
+enable_frame_pointer
 enable_win32_registry
 enable_static
 with_pic
@@ -1597,6 +1598,7 @@  Optional Features:
   --enable-leading-mingw64-underscores
                           Enable leading underscores on 64 bit mingw targets
   --enable-cld            enable -mcld by default for 32bit x86
+  --enable-frame-pointer  enable -fno-omit-frame-pointer by default for 32bit x86
   --disable-win32-registry
                           disable lookup of installation paths in the
                           Registry on Windows hosts
@@ -10708,6 +10710,24 @@  else
 fi
 
 
+# Check whether --enable-frame-pointer was given.
+if test "${enable_frame_pointer+set}" = set; then :
+  enableval=$enable_frame_pointer;
+else
+
+case $target_os in
+linux*)
+  # Enable -fomit-frame-pointer by default for Linux.
+  enable_frame_pointer=no
+  ;;
+*)
+  enable_frame_pointer=yes
+  ;;
+esac
+
+fi
+
+
 # Windows32 Registry support for specifying GCC installation paths.
 # Check whether --enable-win32-registry was given.
 if test "${enable_win32_registry+set}" = set; then :
@@ -17109,7 +17129,7 @@  else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 17112 "configure"
+#line 17132 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
@@ -17215,7 +17235,7 @@  else
   lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
   lt_status=$lt_dlunknown
   cat > conftest.$ac_ext <<_LT_EOF
-#line 17218 "configure"
+#line 17238 "configure"
 #include "confdefs.h"
 
 #if HAVE_DLFCN_H
diff --git a/gcc/configure.ac b/gcc/configure.ac
index 24d38aa..dd0b198 100644
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -1580,6 +1580,20 @@  AC_ARG_ENABLE(cld,
 [  --enable-cld            enable -mcld by default for 32bit x86], [],
 [enable_cld=no])
 
+AC_ARG_ENABLE(frame-pointer,
+[  --enable-frame-pointer  enable -fno-omit-frame-pointer by default for 32bit x86], [],
+[
+case $target_os in
+linux*)
+  # Enable -fomit-frame-pointer by default for Linux.
+  enable_frame_pointer=no
+  ;;
+*)
+  enable_frame_pointer=yes
+  ;;
+esac
+])
+
 # Windows32 Registry support for specifying GCC installation paths.
 AC_ARG_ENABLE(win32-registry,
 [  --disable-win32-registry