Re: var-tracking and s390's LM(G)

[Richard Sandiford <rsandifo at linux dot vnet dot ibm dot com>]

Richard Henderson <rth@redhat.com> writes:
> On 02/05/2014 02:30 PM, Ulrich Weigand wrote:
>> Jakub Jelinek wrote:
>>> On Wed, Feb 05, 2014 at 10:26:16PM +0100, Ulrich Weigand wrote:
>>>> Actually, now I think the problem originally described there is still
>>>> valid: on s390 the CFA is *not* equal to the value at function entry,
>>>> but biased by 96/160 bytes.  So setting the SP to the CFA is wrong ...
>>>
>>> Such biasing happens on most of the targets, e.g. x86_64 has upon function
>>> entry CFA set to %rsp + 8, i?86 to %esp + 4.
>> 
>> Ah, but that's a different bias.  In those cases it is still correct
>> to *unwind* SP to the CFA, since that's the value SP needs to have
>> back in the caller immediately after the call site.
>> 
>> On s390, the call/return instructions do not modify the SP so the
>> SP at function entry is equal to the SP in the caller after return,
>> but neither of these is equal to the CFA.  Instead, SP points to
>> 96/160 bytes below the CFA ...   Therefore you cannot simply
>> unwind SP to the CFA.
>
> I was about to reply the same to Jakub, Ulrich beat me to it.
>
> Basically, the CFA has been defined "incorrectly" for s390, but it hasn't
> mattered since they record the save of the SP.  But the result is that you
> can't stop recording the save of SP without also adjusting how the CFA
> is defined.
>
> Which _can_ be done, in a way that's fully compatible with all of the existing
> unwinding.  But certainly shouldn't be attempted at this stage of development.

OK, I agree that's not 4.9 material.  What about the other change
of replacing:

   REF_CFA_DEF_CFA (plus (stack_pointer_rtx) (const_int 160/96))

with:

   REF_CFA_ADJUST_CFA (set (stack_pointer_rtx)
                           (plus (current_cfa_base) (const_int offset)))

?  That works on its own, but having both a REG_CFA_ADJUST_CFA that assigns
to stack_pointer_rtx and a REG_CFA_RESTORE for stack_pointer_rtx feels like
a double assignment.  Personally I'd prefer to leave the REG_CFA_DEF_CFA
as-is for now and change all three (the save, the restore and the CFA
definition) at the same time.

I'm also a bit worried about handling REG_CFA_ADJUST_CFA in
var-tracking.c at this stage since AIUI it used to be valid for
a backend to use CFA notes to summarise the effect of several
instructions, with only the last of them being marked frame-related.
If we look at non-frame-related insn patterns as well as CFA notes
then we could end up double-counting.  But I suppose the same goes for
REG_FRAME_RELATED_EXPRs and insn_stack_adjust_offset_pre_post has been
using those without any known problems.

FWIW here's the patch I tested.

Thanks,
Richard


gcc/
	* var-tracking.c (insn_stack_adjust_offset_pre_post): Handle
	REG_CFA_ADJUST_CFA.
	* config/s390/s390.c (s390_add_restore_cfa_note): New function.
	(s390_emit_epilogue): Use it.

Index: gcc/var-tracking.c
===================================================================
--- gcc/var-tracking.c	2014-02-06 09:47:55.564375661 +0000
+++ gcc/var-tracking.c	2014-02-06 09:47:57.259364367 +0000
@@ -807,6 +807,14 @@ insn_stack_adjust_offset_pre_post (rtx i
       rtx expr = find_reg_note (insn, REG_FRAME_RELATED_EXPR, NULL_RTX);
       if (expr)
 	pattern = XEXP (expr, 0);
+      else
+	{
+	  expr = find_reg_note (insn, REG_CFA_ADJUST_CFA, NULL_RTX);
+	  if (expr
+	      && XEXP (expr, 0)
+	      && SET_DEST (XEXP (expr, 0)) == stack_pointer_rtx)
+	    pattern = XEXP (expr, 0);
+	}
     }
 
   if (GET_CODE (pattern) == SET)
Index: gcc/config/s390/s390.c
===================================================================
--- gcc/config/s390/s390.c	2014-02-06 09:47:55.564375661 +0000
+++ gcc/config/s390/s390.c	2014-02-06 09:48:47.378031557 +0000
@@ -8775,6 +8775,22 @@ s390_emit_stack_tie (void)
   emit_insn (gen_stack_tie (mem));
 }
 
+/* INSN is the epilogue instruction that sets the stack pointer to its
+   final end-of-function value.  Add a REG_CFA_ADJUST_CFA to INSN to
+   describe that final value.  FP_OFFSET is the amount that needs to be
+   added to the current hard frame pointer or stack pointer in order to
+   get the final value.  */
+
+static void
+s390_add_restore_cfa_note (rtx insn, HOST_WIDE_INT fp_offset)
+{
+  rtx base = frame_pointer_needed ? hard_frame_pointer_rtx : stack_pointer_rtx;
+  if (base != stack_pointer_rtx || fp_offset != 0)
+    add_reg_note (insn, REG_CFA_ADJUST_CFA,
+		  gen_rtx_SET (VOIDmode, stack_pointer_rtx,
+			       plus_constant (Pmode, base, fp_offset)));
+}
+
 /* Copy GPRS into FPR save slots.  */
 
 static void
@@ -9316,9 +9332,7 @@ s390_emit_epilogue (bool sibcall)
 			   cfun_frame_layout.last_restore_gpr);
       insn = emit_insn (insn);
       REG_NOTES (insn) = cfa_restores;
-      add_reg_note (insn, REG_CFA_DEF_CFA,
-		    plus_constant (Pmode, stack_pointer_rtx,
-				   STACK_POINTER_OFFSET));
+      s390_add_restore_cfa_note (insn, offset);
       RTX_FRAME_RELATED_P (insn) = 1;
     }