Richard Henderson <r...@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;
}
