Patchwork [PR,60580,AArch64] Fix __attribute__ ((optimize("no-omit-frame-pointer")))

login
register
mail settings
Submitter Marcus Shawcroft
Date March 25, 2014, 8:51 a.m.
Message ID <5331438F.5060605@arm.com>
Download mbox | patch
Permalink /patch/333330/
State New
Headers show

Comments

Marcus Shawcroft - March 25, 2014, 8:51 a.m.
Hi,

The implementation of -m[no-]omit-leaf-frame-pointer and 
-f[no-]omit-frame-pointer in the AArch64 target does not behave 
correctly in the presence of __attribute__ optimize.

This patch adjusts the implementation to work in a similar fashion to 
the same functionality in the i386 target.

The problem occurs because the current implementation uses a global 
'faked_omit_frame_pointer' to retain the original value of 
flag_omit_frame_pointer.  The global does not form part of the 
optimization save state.

This solution removes the global and instead tracks required behaviour 
using only flag_omit_frame_pointer and flag_omit_leaf_frame_pointer. 
These two form part of the optimziation save state and target save state 
respectively.

The additional complication for AArch64 is that the PCS requires that 
given -fno-omit-frame-pointer -momit-leave-frame-pointer, a leaf 
function that kills LR must create a frame record.  This is readily 
handled in aarch64_frame_pointer_required().  I've dropped logic in 
aarch64_can_eliminate() that attempts to detect this scenario since it 
appears to me to be superflous.

I'll leave this on the list for 24h for folks to comment and to give the 
RM's the chance to object before committing.

Cheers
/Marcus

2014-03-25  Marcus Shawcroft  <marcus.shawcroft@arm.com>

	PR target/60580
	* config/aarch64/aarch64.c (faked_omit_frame_pointer): Remove.
	(aarch64_frame_pointer_required): Adjust logic.
	(aarch64_can_eliminate): Adjust logic.
	(aarch64_override_options_after_change): Adjust logic.

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index d59ecf7..7bd608c 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,11 @@ 
+2014-03-21  Marcus Shawcroft  <marcus.shawcroft@arm.com>
+
+	PR target/60580
+	* config/aarch64/aarch64.c (faked_omit_frame_pointer): Remove.
+	(aarch64_frame_pointer_required): Adjust logic.
+	(aarch64_can_eliminate): Adjust logic.
+	(aarch64_override_options_after_change): Adjust logic.
+
 2014-03-19 Venkataramanan Kumar  <venkataramanan.kumar@linaro.org>
 
 	* config/aarch64/aarch64.md (stack_protect_set, stack_protect_test)
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index a0b20b67..c360289 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -315,10 +315,6 @@  static GTY(()) int gty_dummy;
 #define AARCH64_NUM_BITMASKS  5334
 static unsigned HOST_WIDE_INT aarch64_bitmasks[AARCH64_NUM_BITMASKS];
 
-/* Did we set flag_omit_frame_pointer just so
-   aarch64_frame_pointer_required would be called? */
-static bool faked_omit_frame_pointer;
-
 typedef enum aarch64_cond_code
 {
   AARCH64_EQ = 0, AARCH64_NE, AARCH64_CS, AARCH64_CC, AARCH64_MI, AARCH64_PL,
@@ -1730,17 +1726,15 @@  aarch64_frame_pointer_required (void)
   if (cfun->calls_alloca)
     return true;
 
-  /* We may have turned flag_omit_frame_pointer on in order to have this
-     function called; if we did, we also set the 'faked_omit_frame_pointer' flag
-     and we'll check it here.
-     If we really did set flag_omit_frame_pointer normally, then we return false
-     (no frame pointer required) in all cases.  */
+  /* In aarch64_override_options_after_change
+     flag_omit_leaf_frame_pointer turns off the frame pointer by
+     default.  Turn it back on now if we've not got a leaf
+     function.  */
+  if (flag_omit_leaf_frame_pointer
+      && (!crtl->is_leaf || df_regs_ever_live_p (LR_REGNUM)))
+    return true;
 
-  if (flag_omit_frame_pointer && !faked_omit_frame_pointer)
-    return false;
-  else if (flag_omit_leaf_frame_pointer)
-    return !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
@@ -4168,23 +4162,8 @@  aarch64_can_eliminate (const int from, const int to)
 	return true;
       if (from == FRAME_POINTER_REGNUM && to == HARD_FRAME_POINTER_REGNUM)
 	return true;
-    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.
-
-	 NOTE: the original value of flag_omit_frame_pointer gets trashed
-	 IFF flag_omit_leaf_frame_pointer is true, so we check the value
-	 of faked_omit_frame_pointer here (which is true when we always
-	 wish to keep non-leaf frame pointers but only wish to keep leaf frame
-	 pointers when LR is clobbered).  */
-      if (to == STACK_POINTER_REGNUM
-	  && df_regs_ever_live_p (LR_REGNUM)
-	  && faked_omit_frame_pointer)
-	return false;
+
+      return false;
     }
 
   return true;
@@ -5314,17 +5293,10 @@  aarch64_override_options (void)
 static void
 aarch64_override_options_after_change (void)
 {
-  faked_omit_frame_pointer = false;
-
-  /* To omit leaf frame pointers, we need to turn flag_omit_frame_pointer on so
-     that aarch64_frame_pointer_required will be called.  We need to remember
-     whether flag_omit_frame_pointer was turned on normally or just faked.  */
-
-  if (flag_omit_leaf_frame_pointer && !flag_omit_frame_pointer)
-    {
-      flag_omit_frame_pointer = true;
-      faked_omit_frame_pointer = true;
-    }
+  if (flag_omit_frame_pointer)
+    flag_omit_leaf_frame_pointer = false;
+  else if (flag_omit_leaf_frame_pointer)
+    flag_omit_frame_pointer = true;
 }
 
 static struct machine_function *
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 8b3c9b1..d97ed95 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,10 @@ 
+2014-03-21  Marcus Shawcroft  <marcus.shawcroft@arm.com>
+
+	PR target/60580
+	* gcc.target/aarch64/pr60580_1.c: New.
+	* gcc.target/aarch64/test_fp_attribute_1.c: New.
+	* gcc.target/aarch64/test_fp_attribute_2.c: New.
+
 2014-03-14  Alex Velenko  <Alex.Velenko@arm.com>
 
 	* gcc.target/aarch64/vdup_lane_1.c: New testcase.
diff --git a/gcc/testsuite/gcc.target/aarch64/pr60580_1.c b/gcc/testsuite/gcc.target/aarch64/pr60580_1.c
new file mode 100644
index 0000000..1adf508
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr60580_1.c
@@ -0,0 +1,45 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O0 -fomit-frame-pointer -fno-inline --save-temps" } */
+
+void
+func_leaf (void)
+{
+  int a = 0;
+}
+
+void
+func_no_leaf (void)
+{
+  int a = 0;
+  func_leaf ();
+}
+
+void
+func1 (void)
+{
+  int a = 0;
+  func_no_leaf ();
+}
+
+/*
+ * This function calls XXX(), which modifies SP. This is incompatible to
+ * -fomit-frame-pointer generated code as SP is used to access the frame.
+ */
+__attribute__ ((optimize("no-omit-frame-pointer")))
+void
+func2 (void)
+{
+  int a = 0;
+  func_no_leaf ();
+}
+
+void
+func3 (void)
+{
+  int a = 0;
+  func_no_leaf ();
+}
+
+/* { dg-final { scan-assembler-times "stp\tx29, x30, \\\[sp, -\[0-9\]+\\\]!" 1 } } */
+
+/* { dg-final { cleanup-saved-temps } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/test_fp_attribute_1.c b/gcc/testsuite/gcc.target/aarch64/test_fp_attribute_1.c
new file mode 100644
index 0000000..7538250
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/test_fp_attribute_1.c
@@ -0,0 +1,26 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O0 -fno-omit-frame-pointer -fno-inline --save-temps" } */
+
+void
+leaf (void)
+{
+  int a = 0;
+}
+
+__attribute__ ((optimize("omit-frame-pointer")))
+void
+non_leaf_1 (void)
+{
+  leaf ();
+}
+
+__attribute__ ((optimize("omit-frame-pointer")))
+void
+non_leaf_2 (void)
+{
+  leaf ();
+}
+
+/* { dg-final { scan-assembler-times "str\tx30, \\\[sp\\\]" 2 } } */
+
+/* { dg-final { cleanup-saved-temps } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/test_fp_attribute_2.c b/gcc/testsuite/gcc.target/aarch64/test_fp_attribute_2.c
new file mode 100644
index 0000000..675091f
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/test_fp_attribute_2.c
@@ -0,0 +1,26 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O0 -fomit-frame-pointer -fno-inline --save-temps" } */
+
+void
+leaf (void)
+{
+  int a = 0;
+}
+
+__attribute__ ((optimize("no-omit-frame-pointer")))
+void
+non_leaf_1 (void)
+{
+  leaf ();
+}
+
+__attribute__ ((optimize("no-omit-frame-pointer")))
+void
+non_leaf_2 (void)
+{
+  leaf ();
+}
+
+/* { dg-final { scan-assembler-times "stp\tx29, x30, \\\[sp, -\[0-9\]+\\\]!" 2 } } */
+
+/* { dg-final { cleanup-saved-temps } } */