diff mbox series

DWARF: Represent hard frame pointer as stack pointer + offset

Message ID CAMe9rOoCNz6-oTZA3UnbGQLKHHu1C+o6PJBkL7Wqj4u7AoGwbQ@mail.gmail.com
State New
Headers show
Series DWARF: Represent hard frame pointer as stack pointer + offset | expand

Commit Message

H.J. Lu Aug. 31, 2018, 7:33 p.m. UTC 
On Thu, Aug 30, 2018 at 10:21 AM, Jason Merrill <jason@redhat.com> wrote:
>
>> r138335 allowed arg_pointer_rtx to be eliminated by either FP or SP,
>> but only when dynamic stack alignment is supported.  In this case,
>> arg_pointer_rtx is eliminated by FP even when frame_pointer_needed
>> is false and there is no dynamic stack alignment at all.
>>
>>> gcc_assert (elim == stack_pointer_rtx || (frame_pointer_needed && elim
>>> == hard_frame_pointer_rtx));
>>>
>>> so as not to allow eliminating to an uninitialized FP.
>>
>> FP isn't uninitialized.  It is initialized the same way as in the case of
>> SUPPORTS_STACK_ALIGNMENT is true.
>
> How is that?  Why would it be initialized when frame_pointer_needed is
> false?  What does it mean for frame_pointer_needed to be false, if
> not, as in the documentation of -fomit-frame-pointer,
>
> "This avoids the instructions to save, set up and restore the frame
> pointer; on many targets it also makes an extra register available."
>
> ?
>

A backend may not set up frame pointer even with -fno-omit-frame-pointer.
In the case of x86,  hard frame pointer can be represented by stack pointer
- UNITS_PER_WORD.

This patch adds hard_frame_pointer_from_stack_pointer_plus_offset and
hard_frame_pointer_offset to rtl_data to allow a backend to represent
hard frame pointer as stack pointer + offset.

OK for trunk?

Comments

Jason Merrill Aug. 31, 2018, 8:32 p.m. UTC | #1 
On Fri, Aug 31, 2018 at 3:33 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Aug 30, 2018 at 10:21 AM, Jason Merrill <jason@redhat.com> wrote:
>>
>>> r138335 allowed arg_pointer_rtx to be eliminated by either FP or SP,
>>> but only when dynamic stack alignment is supported.  In this case,
>>> arg_pointer_rtx is eliminated by FP even when frame_pointer_needed
>>> is false and there is no dynamic stack alignment at all.
>>>
>>>> gcc_assert (elim == stack_pointer_rtx || (frame_pointer_needed && elim
>>>> == hard_frame_pointer_rtx));
>>>>
>>>> so as not to allow eliminating to an uninitialized FP.
>>>
>>> FP isn't uninitialized.  It is initialized the same way as in the case of
>>> SUPPORTS_STACK_ALIGNMENT is true.
>>
>> How is that?  Why would it be initialized when frame_pointer_needed is
>> false?  What does it mean for frame_pointer_needed to be false, if
>> not, as in the documentation of -fomit-frame-pointer,
>>
>> "This avoids the instructions to save, set up and restore the frame
>> pointer; on many targets it also makes an extra register available."
>>
>> ?
>
> A backend may not set up frame pointer even with -fno-omit-frame-pointer.
> In the case of x86,  hard frame pointer can be represented by stack pointer
> - UNITS_PER_WORD.
>
> This patch adds hard_frame_pointer_from_stack_pointer_plus_offset and
> hard_frame_pointer_offset to rtl_data to allow a backend to represent
> hard frame pointer as stack pointer + offset.

Shouldn't this be fixed in eliminate_regs rather than dwarf2out?

Jason
H.J. Lu Aug. 31, 2018, 9:54 p.m. UTC | #2 
On Fri, Aug 31, 2018 at 1:32 PM, Jason Merrill <jason@redhat.com> wrote:
> On Fri, Aug 31, 2018 at 3:33 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Thu, Aug 30, 2018 at 10:21 AM, Jason Merrill <jason@redhat.com> wrote:
>>>
>>>> r138335 allowed arg_pointer_rtx to be eliminated by either FP or SP,
>>>> but only when dynamic stack alignment is supported.  In this case,
>>>> arg_pointer_rtx is eliminated by FP even when frame_pointer_needed
>>>> is false and there is no dynamic stack alignment at all.
>>>>
>>>>> gcc_assert (elim == stack_pointer_rtx || (frame_pointer_needed && elim
>>>>> == hard_frame_pointer_rtx));
>>>>>
>>>>> so as not to allow eliminating to an uninitialized FP.
>>>>
>>>> FP isn't uninitialized.  It is initialized the same way as in the case of
>>>> SUPPORTS_STACK_ALIGNMENT is true.
>>>
>>> How is that?  Why would it be initialized when frame_pointer_needed is
>>> false?  What does it mean for frame_pointer_needed to be false, if
>>> not, as in the documentation of -fomit-frame-pointer,
>>>
>>> "This avoids the instructions to save, set up and restore the frame
>>> pointer; on many targets it also makes an extra register available."
>>>
>>> ?
>>
>> A backend may not set up frame pointer even with -fno-omit-frame-pointer.
>> In the case of x86,  hard frame pointer can be represented by stack pointer
>> - UNITS_PER_WORD.
>>
>> This patch adds hard_frame_pointer_from_stack_pointer_plus_offset and
>> hard_frame_pointer_offset to rtl_data to allow a backend to represent
>> hard frame pointer as stack pointer + offset.
>
> Shouldn't this be fixed in eliminate_regs rather than dwarf2out?
>

With -fno-omit-frame-pointer, arg pointer is eliminated with hard frame
pointer.  But

commit cd557ff63f388ad27c376d0a225e74d3594a6f9d
Author: hjl <hjl@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Thu Aug 10 15:29:05 2017 +0000

    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.

changed it in the last minute.  It is too late to go back.  When it is done,
hard frame pointer must be replaced by stack pointer - UNITS_PER_WORD
if it is ever used.
Segher Boessenkool Sept. 1, 2018, 11:38 a.m. UTC | #3 
Hi!

On Fri, Aug 31, 2018 at 02:54:17PM -0700, H.J. Lu wrote:
> On Fri, Aug 31, 2018 at 1:32 PM, Jason Merrill <jason@redhat.com> wrote:
> > On Fri, Aug 31, 2018 at 3:33 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> >> On Thu, Aug 30, 2018 at 10:21 AM, Jason Merrill <jason@redhat.com> wrote:
> >>>
> >>>> r138335 allowed arg_pointer_rtx to be eliminated by either FP or SP,
> >>>> but only when dynamic stack alignment is supported.  In this case,
> >>>> arg_pointer_rtx is eliminated by FP even when frame_pointer_needed
> >>>> is false and there is no dynamic stack alignment at all.
> >>>>
> >>>>> gcc_assert (elim == stack_pointer_rtx || (frame_pointer_needed && elim
> >>>>> == hard_frame_pointer_rtx));
> >>>>>
> >>>>> so as not to allow eliminating to an uninitialized FP.
> >>>>
> >>>> FP isn't uninitialized.  It is initialized the same way as in the case of
> >>>> SUPPORTS_STACK_ALIGNMENT is true.
> >>>
> >>> How is that?  Why would it be initialized when frame_pointer_needed is
> >>> false?  What does it mean for frame_pointer_needed to be false, if
> >>> not, as in the documentation of -fomit-frame-pointer,
> >>>
> >>> "This avoids the instructions to save, set up and restore the frame
> >>> pointer; on many targets it also makes an extra register available."
> >>>
> >>> ?
> >>
> >> A backend may not set up frame pointer even with -fno-omit-frame-pointer.
> >> In the case of x86,  hard frame pointer can be represented by stack pointer
> >> - UNITS_PER_WORD.
> >>
> >> This patch adds hard_frame_pointer_from_stack_pointer_plus_offset and
> >> hard_frame_pointer_offset to rtl_data to allow a backend to represent
> >> hard frame pointer as stack pointer + offset.
> >
> > Shouldn't this be fixed in eliminate_regs rather than dwarf2out?
> >
> 
> With -fno-omit-frame-pointer, arg pointer is eliminated with hard frame
> pointer.  But
> 
> commit cd557ff63f388ad27c376d0a225e74d3594a6f9d
> Author: hjl <hjl@138bc75d-0d04-0410-961f-82ee72b054a4>
> Date:   Thu Aug 10 15:29:05 2017 +0000
> 
>     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.
> 
> changed it in the last minute.  It is too late to go back.  When it is done,
> hard frame pointer must be replaced by stack pointer - UNITS_PER_WORD
> if it is ever used.

So after that patch something uses the hard frame pointer, while it also
claims nothing uses the hard frame pointer?  Sounds to me you should fix
the uses, and all will be fine.


Segher
Michael Matz Sept. 3, 2018, 1:56 p.m. UTC | #4 
Hi,

On Sat, 1 Sep 2018, Segher Boessenkool wrote:

> > With -fno-omit-frame-pointer, arg pointer is eliminated with hard frame
> > pointer.  But
> > 
> > commit cd557ff63f388ad27c376d0a225e74d3594a6f9d
> > Author: hjl <hjl@138bc75d-0d04-0410-961f-82ee72b054a4>
> > Date:   Thu Aug 10 15:29:05 2017 +0000
> > 
> >     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.
> > 
> > changed it in the last minute.  It is too late to go back.  When it is done,
> > hard frame pointer must be replaced by stack pointer - UNITS_PER_WORD
> > if it is ever used.
> 
> So after that patch something uses the hard frame pointer, while it also
> claims nothing uses the hard frame pointer?  Sounds to me you should fix
> the uses, and all will be fine.

I and others objected to the above patch (cd557ff63) for a reason as 
ignoring users wishes (see 
https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00466.html and thread).  
Alas, instead of fixing the problems with keeping a useless frame 
pointer setup it was used as a reason to make -fno-omit-frame-pointer even 
more useless :-/


Ciao,
Michael.
diff mbox series

Patch

From 6fc2b6ca9ca77afa2057d2d48ec8f8131036100e Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 8 Aug 2018 06:20:27 -0700
Subject: [PATCH] DWARF: Represent hard frame pointer as stack pointer + offset

With

commit cd557ff63f388ad27c376d0a225e74d3594a6f9d
Author: hjl <hjl@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Thu Aug 10 15:29:05 2017 +0000

    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.

frame pointer may not be available even if -fno-omit-frame-pointer is
used.  When this happened, arg pointer and frame pointer may be eliminated
by hard frame pointer.  In this case, hard frame pointer may be represented
by stack pointer + offset.

This patch adds hard_frame_pointer_from_stack_pointer_plus_offset and
hard_frame_pointer_offset to rtl_data to allow a backend to represent
hard frame pointer as stack pointer + offset.

gcc/

	PR debug/86593
	* dwarf2out.c (based_loc_descr): When frame pointer isn't
	used, if arg pointer or frame pointer are eliminated by hard
	frame pointer, use stack pointer + offset to access stack
	variables if possible.  Update gcc_assert.
	* emit-rtl.h (rtl_data): Add hard_frame_pointer_offset and
	hard_frame_pointer_from_stack_pointer_plus_offset.
	* config/i386/i386.c (ix86_finalize_stack_frame_flags): Set
	hard_frame_pointer_from_stack_pointer_plus_offset and
	hard_frame_pointer_offset to represent hard frame pointer as
	stack pointer - UNITS_PER_WORD if frame pointer isn't used.

gcc/testsuite/

	PR debug/86593
	* g++.dg/pr86593.C: New test.
---
 gcc/config/i386/i386.c         |  6 ++++--
 gcc/dwarf2out.c                | 13 +++++++++++--
 gcc/emit-rtl.h                 |  8 ++++++++
 gcc/testsuite/g++.dg/pr86593.C | 11 +++++++++++
 4 files changed, 34 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/pr86593.C

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 8672a666024..775b96473b1 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -13161,10 +13161,12 @@  ix86_finalize_stack_frame_flags (void)
 	  df_compute_regs_ever_live (true);
 	  df_analyze ();
 
+	  /* Since frame pointer is no longer available, replace it with
+	     stack pointer - UNITS_PER_WORD in debug insns.  */
+	  crtl->hard_frame_pointer_from_stack_pointer_plus_offset = true;
+	  crtl->hard_frame_pointer_offset = -UNITS_PER_WORD;
 	  if (flag_var_tracking)
 	    {
-	      /* Since frame pointer is no longer available, replace it with
-		 stack pointer - UNITS_PER_WORD in debug insns.  */
 	      df_ref ref, next;
 	      for (ref = DF_REG_USE_CHAIN (HARD_FRAME_POINTER_REGNUM);
 		   ref; ref = next)
diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c
index 77317ed2575..09f3c192129 100644
--- a/gcc/dwarf2out.c
+++ b/gcc/dwarf2out.c
@@ -14326,9 +14326,18 @@  based_loc_descr (rtx reg, poly_int64 offset,
       if (elim != reg)
 	{
 	  elim = strip_offset_and_add (elim, &offset);
+
+	  if (!frame_pointer_needed
+	      && elim == hard_frame_pointer_rtx
+	      && crtl->hard_frame_pointer_from_stack_pointer_plus_offset)
+	    {
+	      int fp_offset = crtl->hard_frame_pointer_offset;
+	      int base_reg = DWARF_FRAME_REGNUM (STACK_POINTER_REGNUM);
+	      return new_reg_loc_descr (base_reg, offset + fp_offset);
+	    }
+
 	  gcc_assert ((SUPPORTS_STACK_ALIGNMENT
-		       && (elim == hard_frame_pointer_rtx
-			   || elim == stack_pointer_rtx))
+		       && elim == stack_pointer_rtx)
 	              || elim == (frame_pointer_needed
 				  ? hard_frame_pointer_rtx
 				  : stack_pointer_rtx));
diff --git a/gcc/emit-rtl.h b/gcc/emit-rtl.h
index f089355aef7..c69730e2900 100644
--- a/gcc/emit-rtl.h
+++ b/gcc/emit-rtl.h
@@ -282,6 +282,14 @@  struct GTY(()) rtl_data {
      pass.  */
   bool bb_reorder_complete;
 
+  /* True if hard frame pointer can be represented as stack pointer
+     + offset in the current function.  */
+  bool hard_frame_pointer_from_stack_pointer_plus_offset;
+
+  /* Offset from stack pointer if hard frame pointer can be represented
+     as stack pointer + offset in the current function.   */
+  int hard_frame_pointer_offset;
+
   /* Like regs_ever_live, but 1 if a reg is set or clobbered from an
      asm.  Unlike regs_ever_live, elements of this array corresponding
      to eliminable regs (like the frame pointer) are set if an asm
diff --git a/gcc/testsuite/g++.dg/pr86593.C b/gcc/testsuite/g++.dg/pr86593.C
new file mode 100644
index 00000000000..f4de0c1166a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/pr86593.C
@@ -0,0 +1,11 @@ 
+// { dg-options "-O -g -fno-omit-frame-pointer" }
+
+struct Foo
+{
+    int bar(int a, int b, int c, int i1, int i2, int i3, int d);
+};
+
+int Foo::bar(int a, int b, int c, int i1, int i2, int i3, int d)
+{
+  return 0;
+}
-- 
2.17.1