Message ID | 87a9e4a7ks.fsf@sandifor-thinkpad.stglab.manchester.uk.ibm.com |
---|---|
State | New |
Headers | show |
On 02/06/2014 01:55 AM, Richard Sandiford wrote: > 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. It does seem like it. I suppose it would be easy to suppress the RESTORE of the stack pointer, without changing the save at all. > 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. Yeah, well... > * 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. Looks fine to me, as far as it goes. r~
Richard Henderson <rth@redhat.com> writes: > On 02/06/2014 01:55 AM, Richard Sandiford wrote: >> 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. > > It does seem like it. I suppose it would be easy to suppress the RESTORE of > the stack pointer, without changing the save at all. But if having a restore in the presence of a save doesn't matter, why do we have restores for the other registers? If the idea is that we never care what the CFI state is after the LM(G) then why not omit all of them? Or do you mean that REG_CFA_ADJUST_CFA would act as a REG_CFA_RESTORE too, if there had been a previous save? Thanks, Richard
On 02/06/2014 06:48 AM, Richard Sandiford wrote: > Richard Henderson <rth@redhat.com> writes: >> On 02/06/2014 01:55 AM, Richard Sandiford wrote: >>> 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. >> >> It does seem like it. I suppose it would be easy to suppress the RESTORE of >> the stack pointer, without changing the save at all. > > But if having a restore in the presence of a save doesn't matter, why do > we have restores for the other registers? If the idea is that we never > care what the CFI state is after the LM(G) then why not omit all of them? > > Or do you mean that REG_CFA_ADJUST_CFA would act as a REG_CFA_RESTORE too, > if there had been a previous save? Hum. Your response does make it clear that I may need more coffee. What I wrote above doesn't really make sense. r~
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; }