[PATCH] S/390: Fix PR89952 incorrect CFI

[PATCH] S/390: Fix PR89952 incorrect CFI

[Andreas Krebbel]

This patch fixes a cases where inconsistent CFI is generated.

After restoring the hard frame pointer (r11) from an FPR we have to
set the CFA register.  In order to be able to set it back to the stack
pointer (r15) we have to make sure that r15 has been restored already.

The patch also adds a scheduler dependency to prevent the instruction
scheduler from swapping the r11 and r15 restore again.

gcc/ChangeLog:

2019-04-17  Andreas Krebbel  <krebbel@linux.ibm.com>

	PR target/89952
	* config/s390/s390.c (s390_restore_gprs_from_fprs): Restore GPRs
    	from FPRs in reverse order.  Generate REG_CFA_DEF_CFA note also
    	for restored hard frame pointer.
	(s390_sched_dependencies_evaluation): Implement new target hook.
	(TARGET_SCHED_DEPENDENCIES_EVALUATION_HOOK): New macro definition.

gcc/testsuite/ChangeLog:

2019-04-17  Andreas Krebbel  <krebbel@linux.ibm.com>

	PR target/89952
	* gcc.target/s390/pr89952.c: New test.
---
 gcc/config/s390/s390.c                  | 62 +++++++++++++++++++++++++++++++--
 gcc/testsuite/gcc.target/s390/pr89952.c | 12 +++++++
 2 files changed, 72 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/s390/pr89952.c

diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index ad8eacd..fc4571d 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -10685,7 +10685,11 @@ s390_restore_gprs_from_fprs (void)
   if (!TARGET_Z10 || !TARGET_HARD_FLOAT || !crtl->is_leaf)
     return;
 
-  for (i = 6; i < 16; i++)
+  /* Restore the GPRs starting with the stack pointer.  That way the
+     stack pointer already has its original value when it comes to
+     restoring the hard frame pointer.  So we can set the cfa reg back
+     to the stack pointer.  */
+  for (i = STACK_POINTER_REGNUM; i >= 6; i--)
     {
       rtx_insn *insn;
 
@@ -10701,7 +10705,13 @@ s390_restore_gprs_from_fprs (void)
 
       df_set_regs_ever_live (i, true);
       add_reg_note (insn, REG_CFA_RESTORE, gen_rtx_REG (DImode, i));
-      if (i == STACK_POINTER_REGNUM)
+
+      /* If either the stack pointer or the frame pointer get restored
+	 set the CFA value to its value at function start.  Doing this
+	 for the frame pointer results in .cfi_def_cfa_register 15
+	 what is ok since if the stack pointer got modified it has
+	 been restored already.  */
+      if (i == STACK_POINTER_REGNUM || i == HARD_FRAME_POINTER_REGNUM)
 	add_reg_note (insn, REG_CFA_DEF_CFA,
 		      plus_constant (Pmode, stack_pointer_rtx,
 				     STACK_POINTER_OFFSET));
@@ -16294,6 +16304,49 @@ s390_case_values_threshold (void)
   return default_case_values_threshold ();
 }
 
+/* Evaluate the insns between HEAD and TAIL and do back-end to install
+   back-end specific dependencies.
+
+   Establish an ANTI dependency between r11 and r15 restores from FPRs
+   to prevent the instructions scheduler from reordering them since
+   this would break CFI.  No further handling in the sched_reorder
+   hook is required since the r11 and r15 restore will never appear in
+   the same ready list with that change.  */
+void
+s390_sched_dependencies_evaluation (rtx_insn *head, rtx_insn *tail)
+{
+  if (!frame_pointer_needed || !epilogue_completed)
+    return;
+
+  while (head != tail && DEBUG_INSN_P (head))
+    head = NEXT_INSN (head);
+
+  rtx_insn *r15_restore = NULL, *r11_restore = NULL;
+
+  for (rtx_insn *insn = tail; insn != head; insn = PREV_INSN (insn))
+    {
+      rtx set = single_set (insn);
+      if (!INSN_P (insn)
+	  || !RTX_FRAME_RELATED_P (insn)
+	  || set == NULL_RTX
+	  || !REG_P (SET_DEST (set))
+	  || !FP_REG_P (SET_SRC (set)))
+	continue;
+
+      if (REGNO (SET_DEST (set)) == HARD_FRAME_POINTER_REGNUM)
+	r11_restore = insn;
+
+      if (REGNO (SET_DEST (set)) == STACK_POINTER_REGNUM)
+	r15_restore = insn;
+    }
+
+  if (r11_restore == NULL || r15_restore == NULL)
+    return;
+  add_dependence (r11_restore, r15_restore, REG_DEP_ANTI);
+}
+
+
+
 /* Initialize GCC target structure.  */
 
 #undef  TARGET_ASM_ALIGNED_HI_OP
@@ -16585,6 +16638,11 @@ s390_case_values_threshold (void)
 #undef TARGET_CASE_VALUES_THRESHOLD
 #define TARGET_CASE_VALUES_THRESHOLD s390_case_values_threshold
 
+#undef TARGET_SCHED_DEPENDENCIES_EVALUATION_HOOK
+#define TARGET_SCHED_DEPENDENCIES_EVALUATION_HOOK \
+  s390_sched_dependencies_evaluation
+
+
 /* Use only short displacement, since long displacement is not available for
    the floating point instructions.  */
 #undef TARGET_MAX_ANCHOR_OFFSET
diff --git a/gcc/testsuite/gcc.target/s390/pr89952.c b/gcc/testsuite/gcc.target/s390/pr89952.c
new file mode 100644
index 0000000..9f48e08
--- /dev/null
+++ b/gcc/testsuite/gcc.target/s390/pr89952.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-march=zEC12 -fno-omit-frame-pointer -Os" } */
+
+
+extern void j(int);
+
+void
+d(int e, long f, int g, int h, int i) {
+  if (h == 5 && i >= 4 && i <= 7)
+    h = e;
+  j(h);
+}
-- 
2.7.4

Re: [PATCH] S/390: Fix PR89952 incorrect CFI

[Robin Dapp]

Hi,

> +   Establish an ANTI dependency between r11 and r15 restores from FPRs
> +   to prevent the instructions scheduler from reordering them since
> +   this would break CFI.  No further handling in the sched_reorder
> +   hook is required since the r11 and r15 restore will never appear in
> +   the same ready list with that change.  */
[..]
> +  if (r11_restore == NULL || r15_restore == NULL)
> +    return;
> +  add_dependence (r11_restore, r15_restore, REG_DEP_ANTI);
> +}

an anti dependency seems an odd choice because r15 would be restored
before r11 (according to the patch's changes in
s390_restore_gprs_from_fprs) and the CFI note for r11 reads
from/requires r15.  This would rather indicate a data dependency from
r15 to r11 but I understand that you don't want the scheduler to assume
there is a real dependency including effects on insn priority and latency.

I looked in haifa-sched.c and sched-ebb.c for methods to prevent
reordering two "special" instructions but did not find anything. There
is of course code to prevent scheduling stack-referencing insns across a
stack pointer restore but it doesn't seem to allow adding more
instructions.

I came across add_deps_for_risky_insns, though, which also uses anti
dependencies, apparently for a similar reason, so maybe that's the
normal and accepted way to do it.

Regards
 Robin

[PATCH] S/390: Fix PR89952 incorrect CFI

[Andreas Krebbel]

This patch fixes a cases where inconsistent CFI is generated.

After restoring the hard frame pointer (r11) from an FPR we have to
set the CFA register.  In order to be able to set it back to the stack
pointer (r15) we have to make sure that r15 has been restored already.

gcc/ChangeLog:

2019-04-09  Andreas Krebbel  <krebbel@linux.ibm.com>

	PR target/89952
	* config/s390/s390.c (s390_restore_gprs_from_fprs): Restore GPRs
	from FPRs in reverse order.  Generate REG_CFA_DEF_CFA note also
	for restored hard frame pointer.

gcc/testsuite/ChangeLog:

2019-04-09  Andreas Krebbel  <krebbel@linux.ibm.com>

	PR target/89952
	* gcc.target/s390/pr89952.c: New test.
---
 gcc/config/s390/s390.c                  | 14 ++++++++++++--
 gcc/testsuite/gcc.target/s390/pr89952.c | 12 ++++++++++++
 2 files changed, 24 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/s390/pr89952.c

diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index e0b62b7..e95ea5b 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -10685,7 +10685,11 @@ s390_restore_gprs_from_fprs (void)
   if (!TARGET_Z10 || !TARGET_HARD_FLOAT || !crtl->is_leaf)
     return;
 
-  for (i = 6; i < 16; i++)
+  /* Restore the GPRs starting with the stack pointer.  That way the
+     stack pointer already has its original value when it comes to
+     restoring the hard frame pointer.  So we can set the cfa reg back
+     to the stack pointer.  */
+  for (i = STACK_POINTER_REGNUM; i >= 6; i--)
     {
       rtx_insn *insn;
 
@@ -10701,7 +10705,13 @@ s390_restore_gprs_from_fprs (void)
 
       df_set_regs_ever_live (i, true);
       add_reg_note (insn, REG_CFA_RESTORE, gen_rtx_REG (DImode, i));
-      if (i == STACK_POINTER_REGNUM)
+
+      /* If either the stack pointer or the frame pointer get restored
+	 set the CFA value to its value at function start.  Doing this
+	 for the frame pointer results in .cfi_def_cfa_register 15
+	 what is ok since if the stack pointer got modified it has
+	 been restored already.  */
+      if (i == STACK_POINTER_REGNUM || i == HARD_FRAME_POINTER_REGNUM)
 	add_reg_note (insn, REG_CFA_DEF_CFA,
 		      plus_constant (Pmode, stack_pointer_rtx,
 				     STACK_POINTER_OFFSET));
diff --git a/gcc/testsuite/gcc.target/s390/pr89952.c b/gcc/testsuite/gcc.target/s390/pr89952.c
new file mode 100644
index 0000000..9f48e08
--- /dev/null
+++ b/gcc/testsuite/gcc.target/s390/pr89952.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-march=zEC12 -fno-omit-frame-pointer -Os" } */
+
+
+extern void j(int);
+
+void
+d(int e, long f, int g, int h, int i) {
+  if (h == 5 && i >= 4 && i <= 7)
+    h = e;
+  j(h);
+}
-- 
2.7.4