Re: var-tracking and s390's LM(G)
On Tue, Feb 04, 2014 at 10:47:49AM +, Richard Sandiford wrote: > I wondered whether we could model the load of the stack pointer as an > addition in cases where that is safe (e.g. it would require !calls_eh_return > at least). But that would only work in functions that don't need a frame > pointer. In functions that do need a frame pointer we'd presumably have > (plus (hfp) (const_int X)) instead, which stack_adjust_offset_pre_post > would again not understand. But vt_stack_adjustments is only called if frame pointer isn't used. > Another option would be to work out the offset from REG_CFA_DEF_CFA notes, > where present, but I'm a bit uncomfortable with the idea of mixing two > different methods of calculating the stack offset. So, how does the lmg insn look like in RTL dump on some problematic testcase? insn_stack_adjust_offset_pre_post already uses REG_FRAME_RELATED_EXPR, which is also a kind of CFI note (the oldest one), so likely the issue is just that it hasn't been adjusted to handle other newer REG_CFA_* notes that tell how the stack pointer is adjusted. > The simplest fix seems to be to disable this check for the exit block. > We never use its stack_adjust anyway, and dwarf2cfi already checks > (using CFA information) that the offsets in a shrink-wrapped function > are consistent. > > Tested on s390-linux-gnu and s390x-linux-gnu. OK to install? I don't like this, my strong preference is to handle REG_CFA_* notes. Jakub
Re: var-tracking and s390's LM(G)
Jakub Jelinek writes: > On Tue, Feb 04, 2014 at 10:47:49AM +, Richard Sandiford wrote: >> I wondered whether we could model the load of the stack pointer as an >> addition in cases where that is safe (e.g. it would require !calls_eh_return >> at least). But that would only work in functions that don't need a frame >> pointer. In functions that do need a frame pointer we'd presumably have >> (plus (hfp) (const_int X)) instead, which stack_adjust_offset_pre_post >> would again not understand. > > But vt_stack_adjustments is only called if frame pointer isn't used. Sorry, missed that. >> Another option would be to work out the offset from REG_CFA_DEF_CFA notes, >> where present, but I'm a bit uncomfortable with the idea of mixing two >> different methods of calculating the stack offset. > > So, how does the lmg insn look like in RTL dump on some problematic > testcase? > insn_stack_adjust_offset_pre_post already uses REG_FRAME_RELATED_EXPR, > which is also a kind of CFI note (the oldest one), so likely the issue > is just that it hasn't been adjusted to handle other newer REG_CFA_* notes > that tell how the stack pointer is adjusted. It's just a (mem ...) access: (parallel [... (set (reg %r14) (mem:[SD]I (plus (reg ...) (const_int X1 (set (reg %r15) (mem:[SD]I (plus (reg ...) (const_int X2]) >> The simplest fix seems to be to disable this check for the exit block. >> We never use its stack_adjust anyway, and dwarf2cfi already checks >> (using CFA information) that the offsets in a shrink-wrapped function >> are consistent. >> >> Tested on s390-linux-gnu and s390x-linux-gnu. OK to install? > > I don't like this, my strong preference is to handle REG_CFA_* notes. But then we wouldn't be able to use var-tracking when __builtin_eh_return is used, since in that case replacing the (set (reg ...) (mem ...)) with a (plus ...) would be incorrect -- the value we're loading from the stack will have had a variable adjustment applied. And I know from painful experience that being able to debug the unwind code is very useful. :-) Thanks, Richard
Re: var-tracking and s390's LM(G)
On Tue, Feb 04, 2014 at 11:33:57AM +, Richard Sandiford wrote: > > So, how does the lmg insn look like in RTL dump on some problematic > > testcase? > > insn_stack_adjust_offset_pre_post already uses REG_FRAME_RELATED_EXPR, > > which is also a kind of CFI note (the oldest one), so likely the issue > > is just that it hasn't been adjusted to handle other newer REG_CFA_* notes > > that tell how the stack pointer is adjusted. > > It's just a (mem ...) access: > > (parallel >[... > (set (reg %r14) (mem:[SD]I (plus (reg ...) (const_int X1 > (set (reg %r15) (mem:[SD]I (plus (reg ...) (const_int X2]) I meant what reg notes it has (and why it doesn't use REG_FRAME_RELATED_EXPR). > >> The simplest fix seems to be to disable this check for the exit block. > >> We never use its stack_adjust anyway, and dwarf2cfi already checks > >> (using CFA information) that the offsets in a shrink-wrapped function > >> are consistent. > >> > >> Tested on s390-linux-gnu and s390x-linux-gnu. OK to install? > > > > I don't like this, my strong preference is to handle REG_CFA_* notes. > > But then we wouldn't be able to use var-tracking when __builtin_eh_return > is used, since in that case replacing the (set (reg ...) (mem ...)) > with a (plus ...) would be incorrect -- the value we're loading from the > stack will have had a variable adjustment applied. And I know from painful > experience that being able to debug the unwind code is very useful. :-) Aren't functions using EH_RETURN typically using frame pointer? And, var-tracking disabling doesn't really mean no debug info, just worse debug info. IMHO the sanity check in var-tracking is worth much more than var-tracking in unwind-dw2.o in the case where you wouldn't use frame pointer. Why doesn't dwarf2cfi ICE on it then when the CFA changes can't be described properly? Jakub
Re: var-tracking and s390's LM(G)
Jakub Jelinek writes: > On Tue, Feb 04, 2014 at 11:33:57AM +, Richard Sandiford wrote: >> > So, how does the lmg insn look like in RTL dump on some problematic >> > testcase? >> > insn_stack_adjust_offset_pre_post already uses REG_FRAME_RELATED_EXPR, >> > which is also a kind of CFI note (the oldest one), so likely the issue >> > is just that it hasn't been adjusted to handle other newer REG_CFA_* notes >> > that tell how the stack pointer is adjusted. >> >> It's just a (mem ...) access: >> >> (parallel >>[... >> (set (reg %r14) (mem:[SD]I (plus (reg ...) (const_int X1 >> (set (reg %r15) (mem:[SD]I (plus (reg ...) (const_int X2]) > > I meant what reg notes it has (and why it doesn't use > REG_FRAME_RELATED_EXPR). It has a REG_CFA_RESTORE for each loaded register. It also has a REG_CFA_DEF_CFA with (plus (sp) (const_int ...)) in lieu of a REG_FRAME_RELATED_EXPR of the form (set (sp) (plus (...) (const_int ...))), which like I say wouldn't always be correct. And I think that's a valid use of REG_CFA_DEF_CFA: /* Attached to insns that are RTX_FRAME_RELATED_P, but are too complex for FRAME_RELATED_EXPR intuition. ... */ REG_NOTE (CFA_DEF_CFA) (although the assignment to %r15 isn't first in the parallel, despite the snipped part of the comment implying that it should be. I don't understand why we'd require that though.) FWIW this form comes from: 2009-06-04 Jakub Jelinek * config/s390/s390.c (global_not_special_regno_p): New static inline. (save_gprs): Don't tell unwinder when a global register is saved. (s390_emit_epilogue): Emit needed epilogue unwind info. >> >> The simplest fix seems to be to disable this check for the exit block. >> >> We never use its stack_adjust anyway, and dwarf2cfi already checks >> >> (using CFA information) that the offsets in a shrink-wrapped function >> >> are consistent. >> >> >> >> Tested on s390-linux-gnu and s390x-linux-gnu. OK to install? >> > >> > I don't like this, my strong preference is to handle REG_CFA_* notes. >> >> But then we wouldn't be able to use var-tracking when __builtin_eh_return >> is used, since in that case replacing the (set (reg ...) (mem ...)) >> with a (plus ...) would be incorrect -- the value we're loading from the >> stack will have had a variable adjustment applied. And I know from painful >> experience that being able to debug the unwind code is very useful. :-) > > Aren't functions using EH_RETURN typically using frame pointer? > And, var-tracking disabling doesn't really mean no debug info, just worse > debug info. IMHO the sanity check in var-tracking is worth much more than > var-tracking in unwind-dw2.o in the case where you wouldn't use frame > pointer. Why doesn't dwarf2cfi ICE on it then when the CFA changes can't be > described properly? Because the CFA information (as provided by REG_CFA... notes) is correct. It's just that var-tracking doesn't use those notes. FWIW, using them was one of the options I mentioned in the original mail. I don't see why you think relaxing this sanity check for the exit block is so bad though. If your argument is that var-tracking must understand every assignment to the stack pointer then I think it should bail out whenever it sees an assignment to sp that it doesn't understand, rather than silently ignoring it. Thanks, Richard
Re: var-tracking and s390's LM(G)
Jakub Jelinek writes: >> But then we wouldn't be able to use var-tracking when __builtin_eh_return >> is used, since in that case replacing the (set (reg ...) (mem ...)) >> with a (plus ...) would be incorrect -- the value we're loading from the >> stack will have had a variable adjustment applied. And I know from painful >> experience that being able to debug the unwind code is very useful. :-) > > Aren't functions using EH_RETURN typically using frame pointer? Sorry, forgot to respond to this bit. On s390, _Unwind_RaiseException and _Unwind_ForcedUnwind don't use a frame pointer, at least not on trunk. %r11 is used as a general scratch register instead. E.g.: 2ba8 <_Unwind_ForcedUnwind>: 2ba8: 90 6f f0 18 stm %r6,%r15,24(%r15) 2bac: 0d d0 basr%r13,%r0 2bae: 60 40 f0 50 std %f4,80(%r15) 2bb2: 60 60 f0 58 std %f6,88(%r15) 2bb6: a7 fa fd f8 ahi %r15,-520 2bba: 58 c0 d0 9e l %r12,158(%r13) 2bbe: 58 10 d0 9a l %r1,154(%r13) 2bc2: 18 b2 lr %r11,%r2 ... 2c10: 98 6f f2 20 lm %r6,%r15,544(%r15) 2c14: 07 f4 br %r4 Thanks, Richard
Re: var-tracking and s390's LM(G)
On Tue, Feb 04, 2014 at 12:21:14PM +, Richard Sandiford wrote:
> Jakub Jelinek writes:
> >> But then we wouldn't be able to use var-tracking when __builtin_eh_return
> >> is used, since in that case replacing the (set (reg ...) (mem ...))
> >> with a (plus ...) would be incorrect -- the value we're loading from the
> >> stack will have had a variable adjustment applied. And I know from painful
> >> experience that being able to debug the unwind code is very useful. :-)
> >
> > Aren't functions using EH_RETURN typically using frame pointer?
>
> Sorry, forgot to respond to this bit. On s390, _Unwind_RaiseException and
> _Unwind_ForcedUnwind don't use a frame pointer, at least not on trunk.
> %r11 is used as a general scratch register instead. E.g.:
>
> 2ba8 <_Unwind_ForcedUnwind>:
> 2ba8: 90 6f f0 18 stm %r6,%r15,24(%r15)
> 2bac: 0d d0 basr%r13,%r0
> 2bae: 60 40 f0 50 std %f4,80(%r15)
> 2bb2: 60 60 f0 58 std %f6,88(%r15)
> 2bb6: a7 fa fd f8 ahi %r15,-520
> 2bba: 58 c0 d0 9e l %r12,158(%r13)
> 2bbe: 58 10 d0 9a l %r1,154(%r13)
> 2bc2: 18 b2 lr %r11,%r2
> ...
> 2c10: 98 6f f2 20 lm %r6,%r15,544(%r15)
> 2c14: 07 f4 br %r4
Looking at pr54693-2.c -O2 -g -m64 -march=z196 (was that the one you meant)
with your patches, I see:
(insn/f:TI 122 30 31 4 (parallel [
(set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
(const_int 80 [0x50])) [3 S8 A8])
(reg:DI 10 %r10))
(set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
(const_int 88 [0x58])) [3 S8 A8])
(reg:DI 11 %r11))
(set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
(const_int 96 [0x60])) [3 S8 A8])
(reg:DI 12 %r12))
(set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
(const_int 104 [0x68])) [3 S8 A8])
(reg:DI 13 %r13))
(set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
(const_int 112 [0x70])) [3 S8 A8])
(reg:DI 14 %r14))
(set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
(const_int 120 [0x78])) [3 S8 A8])
(reg/f:DI 15 %r15))
]) pr54693-2.c:16 94 {*store_multiple_di}
(expr_list:REG_DEAD (reg:DI 14 %r14)
(expr_list:REG_DEAD (reg:DI 13 %r13)
(expr_list:REG_DEAD (reg:DI 12 %r12)
(expr_list:REG_DEAD (reg:DI 11 %r11)
(expr_list:REG_DEAD (reg:DI 10 %r10)
(nil)))
...
(insn/f 123 31 124 4 (set (reg/f:DI 15 %r15)
(plus:DI (reg/f:DI 15 %r15)
(const_int -160 [0xff60]))) pr54693-2.c:16 65 {*la_64}
(expr_list:REG_FRAME_RELATED_EXPR (set (reg/f:DI 15 %r15)
(plus:DI (reg/f:DI 15 %r15)
(const_int -160 [0xff60])))
(nil)))
...
(insn/f:TI 135 134 136 7 (parallel [
(set (reg:DI 10 %r10)
(mem:DI (plus:DI (reg/f:DI 15 %r15)
(const_int 240 [0xf0])) [3 S8 A8]))
(set (reg:DI 11 %r11)
(mem:DI (plus:DI (reg/f:DI 15 %r15)
(const_int 248 [0xf8])) [3 S8 A8]))
(set (reg:DI 12 %r12)
(mem:DI (plus:DI (reg/f:DI 15 %r15)
(const_int 256 [0x100])) [3 S8 A8]))
(set (reg:DI 13 %r13)
(mem:DI (plus:DI (reg/f:DI 15 %r15)
(const_int 264 [0x108])) [3 S8 A8]))
(set (reg:DI 14 %r14)
(mem:DI (plus:DI (reg/f:DI 15 %r15)
(const_int 272 [0x110])) [3 S8 A8]))
(set (reg/f:DI 15 %r15)
(mem:DI (plus:DI (reg/f:DI 15 %r15)
(const_int 280 [0x118])) [3 S8 A8]))
]) pr54693-2.c:25 92 {*load_multiple_di}
(expr_list:REG_CFA_DEF_CFA (plus:DI (reg/f:DI 15 %r15)
(const_int 160 [0xa0]))
(expr_list:REG_CFA_RESTORE (reg/f:DI 15 %r15)
(expr_list:REG_CFA_RESTORE (reg:DI 14 %r14)
(expr_list:REG_CFA_RESTORE (reg:DI 13 %r13)
(expr_list:REG_CFA_RESTORE (reg:DI 12 %r12)
(expr_list:REG_CFA_RESTORE (reg:DI 11 %r11)
(expr_list:REG_CFA_RESTORE (reg:DI 10 %r10)
(nil)
In a function that doesn't need frame pointer, I'd say this is a serious
bloat of the unwind info, you are saving/restoring %r15 not because you have
to, but just that it seems to be cheaper for the target. So, I'd say you
shouldn't emit DW_CFA_offset 15, 5 on the insn 122 in the prologue and
DW_CFA_restore 15 in the epilogue (twice in this case), that just wast
Re: var-tracking and s390's LM(G)
Jakub Jelinek writes:
> On Tue, Feb 04, 2014 at 12:21:14PM +, Richard Sandiford wrote:
>> Jakub Jelinek writes:
>> >> But then we wouldn't be able to use var-tracking when __builtin_eh_return
>> >> is used, since in that case replacing the (set (reg ...) (mem ...))
>> >> with a (plus ...) would be incorrect -- the value we're loading from the
>> >> stack will have had a variable adjustment applied. And I know from
>> >> painful
>> >> experience that being able to debug the unwind code is very useful. :-)
>> >
>> > Aren't functions using EH_RETURN typically using frame pointer?
>>
>> Sorry, forgot to respond to this bit. On s390, _Unwind_RaiseException and
>> _Unwind_ForcedUnwind don't use a frame pointer, at least not on trunk.
>> %r11 is used as a general scratch register instead. E.g.:
>>
>> 2ba8 <_Unwind_ForcedUnwind>:
>> 2ba8: 90 6f f0 18 stm %r6,%r15,24(%r15)
>> 2bac: 0d d0 basr%r13,%r0
>> 2bae: 60 40 f0 50 std %f4,80(%r15)
>> 2bb2: 60 60 f0 58 std %f6,88(%r15)
>> 2bb6: a7 fa fd f8 ahi %r15,-520
>> 2bba: 58 c0 d0 9e l %r12,158(%r13)
>> 2bbe: 58 10 d0 9a l %r1,154(%r13)
>> 2bc2: 18 b2 lr %r11,%r2
>> ...
>> 2c10: 98 6f f2 20 lm %r6,%r15,544(%r15)
>> 2c14: 07 f4 br %r4
>
> Looking at pr54693-2.c -O2 -g -m64 -march=z196 (was that the one you meant)
> with your patches, I see:
> (insn/f:TI 122 30 31 4 (parallel [
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 80 [0x50])) [3 S8 A8])
> (reg:DI 10 %r10))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 88 [0x58])) [3 S8 A8])
> (reg:DI 11 %r11))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 96 [0x60])) [3 S8 A8])
> (reg:DI 12 %r12))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 104 [0x68])) [3 S8 A8])
> (reg:DI 13 %r13))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 112 [0x70])) [3 S8 A8])
> (reg:DI 14 %r14))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 120 [0x78])) [3 S8 A8])
> (reg/f:DI 15 %r15))
> ]) pr54693-2.c:16 94 {*store_multiple_di}
> (expr_list:REG_DEAD (reg:DI 14 %r14)
> (expr_list:REG_DEAD (reg:DI 13 %r13)
> (expr_list:REG_DEAD (reg:DI 12 %r12)
> (expr_list:REG_DEAD (reg:DI 11 %r11)
> (expr_list:REG_DEAD (reg:DI 10 %r10)
> (nil)))
> ...
> (insn/f 123 31 124 4 (set (reg/f:DI 15 %r15)
> (plus:DI (reg/f:DI 15 %r15)
> (const_int -160 [0xff60]))) pr54693-2.c:16 65 {*la_64}
> (expr_list:REG_FRAME_RELATED_EXPR (set (reg/f:DI 15 %r15)
> (plus:DI (reg/f:DI 15 %r15)
> (const_int -160 [0xff60])))
> (nil)))
> ...
> (insn/f:TI 135 134 136 7 (parallel [
> (set (reg:DI 10 %r10)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 240 [0xf0])) [3 S8 A8]))
> (set (reg:DI 11 %r11)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 248 [0xf8])) [3 S8 A8]))
> (set (reg:DI 12 %r12)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 256 [0x100])) [3 S8 A8]))
> (set (reg:DI 13 %r13)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 264 [0x108])) [3 S8 A8]))
> (set (reg:DI 14 %r14)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 272 [0x110])) [3 S8 A8]))
> (set (reg/f:DI 15 %r15)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 280 [0x118])) [3 S8 A8]))
> ]) pr54693-2.c:25 92 {*load_multiple_di}
> (expr_list:REG_CFA_DEF_CFA (plus:DI (reg/f:DI 15 %r15)
> (const_int 160 [0xa0]))
> (expr_list:REG_CFA_RESTORE (reg/f:DI 15 %r15)
> (expr_list:REG_CFA_RESTORE (reg:DI 14 %r14)
> (expr_list:REG_CFA_RESTORE (reg:DI 13 %r13)
> (expr_list:REG_CFA_RESTORE (reg:DI 12 %r12)
> (expr_list:REG_CFA_RESTORE (reg:DI 11 %r11)
> (expr_list:REG_CFA_RESTORE (reg:DI 10 %r10)
> (nil)
> In a function that doesn't need frame pointer, I'd say this is a serious
> bloat of the unwind info, you are saving/restoring %r15 not because you have
> to, but just that i
Re: var-tracking and s390's LM(G)
On Tue, Feb 04, 2014 at 03:38:51PM +, Richard Sandiford wrote: > What do you want var-tracking to do about the __builtin_eh_return case > though? It isn't correct to say that the instruction adds the frame size > to the stack pointer then, since the loaded value includes the eh_return > adjustment. Pretending it is a constant addition in order to satisfy the > sanity check seems a bit hackish, but there again it'd be a shame to lose > tracking for the whole function just because of that. For __builtin_eh_return the unwind info starting with restoring the stack pointer to the landing pad's stack pointer until you actually return/jump to the landing pad is wrong in either case, not just on s390, but on various other targets. Saying REG_CFA_ADJUST_CFA by the frame size will result in CFA from that point being stack_pointer + 160, which is exactly what you are doing anyway, the DW_CFA_offset 15, ... and DW_CFA_restore aren't helpful to that at all and don't change anything on it. Unless you have the old value of the stack pointer saved in some hard register, providing correct unwind info there is not possible. Jakub
Re: var-tracking and s390's LM(G)
Jakub Jelinek writes: > On Tue, Feb 04, 2014 at 03:38:51PM +, Richard Sandiford wrote: >> What do you want var-tracking to do about the __builtin_eh_return case >> though? It isn't correct to say that the instruction adds the frame size >> to the stack pointer then, since the loaded value includes the eh_return >> adjustment. Pretending it is a constant addition in order to satisfy the >> sanity check seems a bit hackish, but there again it'd be a shame to lose >> tracking for the whole function just because of that. > > For __builtin_eh_return the unwind info starting with restoring the stack > pointer to the landing pad's stack pointer until you actually return/jump to > the > landing pad is wrong in either case, not just on s390, but on various other > targets. Saying REG_CFA_ADJUST_CFA by the frame size will result in CFA > from that point being stack_pointer + 160, which is exactly what you are > doing anyway, the DW_CFA_offset 15, ... and DW_CFA_restore aren't helpful to > that at all and don't change anything on it. > > Unless you have the old value of the stack pointer saved in some hard > register, providing correct unwind info there is not possible. Right, that was my point. So by relying on unwind info in var-tracking.c we're getting the wrong answer for the stack offset after the LM(G) instruction. It sounds like we're going to pretend it's right anyway for expediency. But in that case why not also allow the exit block offsets to vary on the same basis? It's not like the current test is an assert -- it's just silently refusing to do any var-tracking on the function if the epilogue isn't fully understood. If these offsets must match on exit for correctness then we should assert rather than silently bailing out. But if they're allowed to vary then we should do our best to carry on. And since the exit block offset is never used, it seems expedient to allow varying offsets in that case. Thanks, Richard
Re: var-tracking and s390's LM(G)
On Tue, Feb 04, 2014 at 04:59:54PM +, Richard Sandiford wrote: > Right, that was my point. So by relying on unwind info in var-tracking.c > we're getting the wrong answer for the stack offset after the LM(G) > instruction. It sounds like we're going to pretend it's right anyway > for expediency. But in that case why not also allow the exit block > offsets to vary on the same basis? It's not like the current test is > an assert -- it's just silently refusing to do any var-tracking on the > function if the epilogue isn't fully understood. > > If these offsets must match on exit for correctness then we should > assert rather than silently bailing out. But if they're allowed to vary > then we should do our best to carry on. And since the exit block offset > is never used, it seems expedient to allow varying offsets in that case. Ok then, but please do follow-up on the changes to not save/restore r15 in the unwind info, use REG_CFA_ADJUST_CFA where possible and teach var-tracking about that note. Jakub
Re: var-tracking and s390's LM(G)
Jakub Jelinek writes: > On Tue, Feb 04, 2014 at 04:59:54PM +, Richard Sandiford wrote: >> Right, that was my point. So by relying on unwind info in var-tracking.c >> we're getting the wrong answer for the stack offset after the LM(G) >> instruction. It sounds like we're going to pretend it's right anyway >> for expediency. But in that case why not also allow the exit block >> offsets to vary on the same basis? It's not like the current test is >> an assert -- it's just silently refusing to do any var-tracking on the >> function if the epilogue isn't fully understood. >> >> If these offsets must match on exit for correctness then we should >> assert rather than silently bailing out. But if they're allowed to vary >> then we should do our best to carry on. And since the exit block offset >> is never used, it seems expedient to allow varying offsets in that case. > > Ok then, but please do follow-up on the changes to not save/restore > r15 in the unwind info, use REG_CFA_ADJUST_CFA where possible and teach > var-tracking about that note. Thanks, will do. I know Uli was worried about the CFI size too. richard
Re: var-tracking and s390's LM(G)
Jakub Jelinek writes:
> On Tue, Feb 04, 2014 at 12:21:14PM +, Richard Sandiford wrote:
>> Jakub Jelinek writes:
>> >> But then we wouldn't be able to use var-tracking when __builtin_eh_return
>> >> is used, since in that case replacing the (set (reg ...) (mem ...))
>> >> with a (plus ...) would be incorrect -- the value we're loading from the
>> >> stack will have had a variable adjustment applied. And I know from
>> >> painful
>> >> experience that being able to debug the unwind code is very useful. :-)
>> >
>> > Aren't functions using EH_RETURN typically using frame pointer?
>>
>> Sorry, forgot to respond to this bit. On s390, _Unwind_RaiseException and
>> _Unwind_ForcedUnwind don't use a frame pointer, at least not on trunk.
>> %r11 is used as a general scratch register instead. E.g.:
>>
>> 2ba8 <_Unwind_ForcedUnwind>:
>> 2ba8: 90 6f f0 18 stm %r6,%r15,24(%r15)
>> 2bac: 0d d0 basr%r13,%r0
>> 2bae: 60 40 f0 50 std %f4,80(%r15)
>> 2bb2: 60 60 f0 58 std %f6,88(%r15)
>> 2bb6: a7 fa fd f8 ahi %r15,-520
>> 2bba: 58 c0 d0 9e l %r12,158(%r13)
>> 2bbe: 58 10 d0 9a l %r1,154(%r13)
>> 2bc2: 18 b2 lr %r11,%r2
>> ...
>> 2c10: 98 6f f2 20 lm %r6,%r15,544(%r15)
>> 2c14: 07 f4 br %r4
>
> Looking at pr54693-2.c -O2 -g -m64 -march=z196 (was that the one you meant)
> with your patches, I see:
> (insn/f:TI 122 30 31 4 (parallel [
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 80 [0x50])) [3 S8 A8])
> (reg:DI 10 %r10))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 88 [0x58])) [3 S8 A8])
> (reg:DI 11 %r11))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 96 [0x60])) [3 S8 A8])
> (reg:DI 12 %r12))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 104 [0x68])) [3 S8 A8])
> (reg:DI 13 %r13))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 112 [0x70])) [3 S8 A8])
> (reg:DI 14 %r14))
> (set/f (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 120 [0x78])) [3 S8 A8])
> (reg/f:DI 15 %r15))
> ]) pr54693-2.c:16 94 {*store_multiple_di}
> (expr_list:REG_DEAD (reg:DI 14 %r14)
> (expr_list:REG_DEAD (reg:DI 13 %r13)
> (expr_list:REG_DEAD (reg:DI 12 %r12)
> (expr_list:REG_DEAD (reg:DI 11 %r11)
> (expr_list:REG_DEAD (reg:DI 10 %r10)
> (nil)))
> ...
> (insn/f 123 31 124 4 (set (reg/f:DI 15 %r15)
> (plus:DI (reg/f:DI 15 %r15)
> (const_int -160 [0xff60]))) pr54693-2.c:16 65 {*la_64}
> (expr_list:REG_FRAME_RELATED_EXPR (set (reg/f:DI 15 %r15)
> (plus:DI (reg/f:DI 15 %r15)
> (const_int -160 [0xff60])))
> (nil)))
> ...
> (insn/f:TI 135 134 136 7 (parallel [
> (set (reg:DI 10 %r10)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 240 [0xf0])) [3 S8 A8]))
> (set (reg:DI 11 %r11)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 248 [0xf8])) [3 S8 A8]))
> (set (reg:DI 12 %r12)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 256 [0x100])) [3 S8 A8]))
> (set (reg:DI 13 %r13)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 264 [0x108])) [3 S8 A8]))
> (set (reg:DI 14 %r14)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 272 [0x110])) [3 S8 A8]))
> (set (reg/f:DI 15 %r15)
> (mem:DI (plus:DI (reg/f:DI 15 %r15)
> (const_int 280 [0x118])) [3 S8 A8]))
> ]) pr54693-2.c:25 92 {*load_multiple_di}
> (expr_list:REG_CFA_DEF_CFA (plus:DI (reg/f:DI 15 %r15)
> (const_int 160 [0xa0]))
> (expr_list:REG_CFA_RESTORE (reg/f:DI 15 %r15)
> (expr_list:REG_CFA_RESTORE (reg:DI 14 %r14)
> (expr_list:REG_CFA_RESTORE (reg:DI 13 %r13)
> (expr_list:REG_CFA_RESTORE (reg:DI 12 %r12)
> (expr_list:REG_CFA_RESTORE (reg:DI 11 %r11)
> (expr_list:REG_CFA_RESTORE (reg:DI 10 %r10)
> (nil)
> In a function that doesn't need frame pointer, I'd say this is a serious
> bloat of the unwind info, you are saving/restoring %r15 not because you have
> to, but just that i
Re: var-tracking and s390's LM(G)
On Wed, Feb 05, 2014 at 03:32:22PM +, Richard Sandiford wrote:
> > In a function that doesn't need frame pointer, I'd say this is a serious
> > bloat of the unwind info, you are saving/restoring %r15 not because you have
> > to, but just that it seems to be cheaper for the target. So, I'd say you
> > shouldn't emit DW_CFA_offset 15, 5 on the insn 122 in the prologue and
> > DW_CFA_restore 15 in the epilogue (twice in this case), that just waste
> > of .eh_frame/.debug_frame space. I'd say you should represent in
> > this case the *store_multiple_di as REG_FRAME_RELATED_EXPR
> > with all but the last set in the PARALLEL, and on the *load_multiple_di
> > remove REG_CFA_RESTORE (r15) note and change REG_CFA_DEF_CFA note to
> > REG_CFA_ADJUST_CFA note which would say that stack pointer has been
> > incremented by 160 bytes (epilogue expansion knows that value).
>
> I think at the moment we're relying on the "DW_CFA_offset 15"s to
> provide correct %r15 values on eh_return. Every non-leaf function
> saves the stack pointer and the unwind routines track the %r15 save
> slots like they would track any call-saved register. In other words,
> the final eh_return %r15 comes directly from a save slot, without any
> CFA-specific handling. If some frames omit the CFI for the %r15 save
> then we'll just keep the save slot from the previous (inner) frame instead.
CCing Richard Henderson into the discussion.
> The new unwind routines would handle 4.9+ and pre-4.9 frames, but pre-4.9
> unwind routines wouldn't handle 4.9+ frames, so would that require a version
> bump on the unwind symbols?
No need to version anything IMHO. We've never done that for any port
where we've started to emit something the old unwinder wouldn't cope with,
it is user's responsibility to use new enough libgcc_s.
BTW, what is the reason why %r15 is saved/restored from the stack at all say
for simple:
void
foo (void)
{
int a = 6;
asm volatile ("# %0" : "+m" (a));
}
Is that some ABI matter where it effectively always uses requires to use
some kind of frame pointer (which the saved previous stack pointer is)?
It can surely make debugging without unwind info easier, ditto backtraces,
on the other side it must have a runtime cost.
Jakub
Re: var-tracking and s390's LM(G)
On 02/05/2014 07:58 AM, Jakub Jelinek wrote:
> On Wed, Feb 05, 2014 at 03:32:22PM +, Richard Sandiford wrote:
>> I think at the moment we're relying on the "DW_CFA_offset 15"s to
>> provide correct %r15 values on eh_return. Every non-leaf function
>> saves the stack pointer and the unwind routines track the %r15 save
>> slots like they would track any call-saved register. In other words,
>> the final eh_return %r15 comes directly from a save slot, without any
>> CFA-specific handling. If some frames omit the CFI for the %r15 save
>> then we'll just keep the save slot from the previous (inner) frame instead.
>
> CCing Richard Henderson into the discussion.
If the sp is saved for a given frame, we'll use that instead of the CFA when
unwinding to a previous frame. That should be unaffected by whether or not sp
is saved for any particular frame.
There appears to be code in uw_install_context_1 to handle a mix of sp-saving
and non-sp-saving frames. Given that s390 is pretty much the only target that
uses these paths, I suppose it could be broken. But if it is, it would surely
be better to fix than just say "it doesn't work".
> BTW, what is the reason why %r15 is saved/restored from the stack at all say
> for simple:
> void
> foo (void)
> {
> int a = 6;
> asm volatile ("# %0" : "+m" (a));
> }
>
> Is that some ABI matter where it effectively always uses requires to use
> some kind of frame pointer (which the saved previous stack pointer is)?
> It can surely make debugging without unwind info easier, ditto backtraces,
> on the other side it must have a runtime cost.
As far as I recall, the return address is in r14 so the ABI also saves r15 for
all non-leaf functions simply because it is practically free to do so with
store-multiple. This creates the familiar ra/fp unwinding chain. Given how
cheap it is to maintain this chain on s390, I see no particular call to break
it.
As for why r15 gets saved for this leaf example... I have no idea.
Seems like a bug to me, frankly.
r~
Re: var-tracking and s390's LM(G)
Richard Henderson writes: > On 02/05/2014 07:58 AM, Jakub Jelinek wrote: >> On Wed, Feb 05, 2014 at 03:32:22PM +, Richard Sandiford wrote: >>> I think at the moment we're relying on the "DW_CFA_offset 15"s to >>> provide correct %r15 values on eh_return. Every non-leaf function >>> saves the stack pointer and the unwind routines track the %r15 save >>> slots like they would track any call-saved register. In other words, >>> the final eh_return %r15 comes directly from a save slot, without any >>> CFA-specific handling. If some frames omit the CFI for the %r15 save >>> then we'll just keep the save slot from the previous (inner) frame instead. >> >> CCing Richard Henderson into the discussion. > > If the sp is saved for a given frame, we'll use that instead of the CFA when > unwinding to a previous frame. That should be unaffected by whether or not sp > is saved for any particular frame. > > There appears to be code in uw_install_context_1 to handle a mix of sp-saving > and non-sp-saving frames. Given that s390 is pretty much the only target that > uses these paths, I suppose it could be broken. But if it is, it would surely > be better to fix than just say "it doesn't work". OK. When I was looking at it earlier today, the reason it didn't work seemed to be that if %r15 wasn't saved for a particular frame, the context would inherit the %r15 save slot for the previous frame, just like when some functions use and save a particular call-saved register and some don't. So we would only get to uw_install_context_1 without a %r15 slot if none of the frames had one. In other words, the reason seemed to be that the _Unwind_SetGRPtr in: #ifdef EH_RETURN_STACKADJ_RTX /* Special handling here: Many machines do not use a frame pointer, and track the CFA only through offsets from the stack pointer from one frame to the next. In this case, the stack pointer is never stored, so it has no saved address in the context. What we do have is the CFA from the previous stack frame. In very special situations (such as unwind info for signal return), there may be location expressions that use the stack pointer as well. Do this conditionally for one frame. This allows the unwind info for one frame to save a copy of the stack pointer from the previous frame, and be able to use much easier CFA mechanisms to do it. Always zap the saved stack pointer value for the next frame; carrying the value over from one frame to another doesn't make sense. */ _Unwind_SpTmp tmp_sp; if (!_Unwind_GetGRPtr (&orig_context, __builtin_dwarf_sp_column ())) _Unwind_SetSpColumn (&orig_context, context->cfa, &tmp_sp); _Unwind_SetGRPtr (context, __builtin_dwarf_sp_column (), NULL); #endif was conditional on EH_RETURN_STACKADJ_RTX being defined. It looked at first glance like things would work if that call was moved outside, so that we never inherit save slots for the stack pointer. Does that make sense? I can give it a go tomorrow if so. Thanks, Richard
Re: var-tracking and s390's LM(G)
Richard Sandiford wrote: > In other words, the reason seemed to be that the _Unwind_SetGRPtr in: > > #ifdef EH_RETURN_STACKADJ_RTX > /* Special handling here: Many machines do not use a frame pointer, > and track the CFA only through offsets from the stack pointer from > one frame to the next. In this case, the stack pointer is never > stored, so it has no saved address in the context. What we do > have is the CFA from the previous stack frame. > > In very special situations (such as unwind info for signal return), > there may be location expressions that use the stack pointer as well. > > Do this conditionally for one frame. This allows the unwind info > for one frame to save a copy of the stack pointer from the previous > frame, and be able to use much easier CFA mechanisms to do it. > Always zap the saved stack pointer value for the next frame; carrying > the value over from one frame to another doesn't make sense. */ > > _Unwind_SpTmp tmp_sp; > > if (!_Unwind_GetGRPtr (&orig_context, __builtin_dwarf_sp_column ())) > _Unwind_SetSpColumn (&orig_context, context->cfa, &tmp_sp); > _Unwind_SetGRPtr (context, __builtin_dwarf_sp_column (), NULL); > #endif > > was conditional on EH_RETURN_STACKADJ_RTX being defined. It looked at > first glance like things would work if that call was moved outside, > so that we never inherit save slots for the stack pointer. Does that > make sense? I can give it a go tomorrow if so. I haven't looked into this in detail right now, but I recall that I had to specifically add that #ifdef a long time ago since unwinding wouldn't work correctly on s390 otherwise: http://gcc.gnu.org/ml/gcc-patches/2003-05/msg00904.html And the background of the bug here: http://gcc.gnu.org/ml/gcc/2003-05/msg00536.html 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 ... Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain ulrich.weig...@de.ibm.com
Re: var-tracking and s390's LM(G)
I wrote and forgot to proof-read: > 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, ... CFA is not equal to the *SP* value at function entry ... > but biased by 96/160 bytes. So setting the SP to the CFA is wrong ... Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain ulrich.weig...@de.ibm.com
Re: var-tracking and s390's LM(G)
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. Jakub
Re: var-tracking and s390's LM(G)
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. Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain ulrich.weig...@de.ibm.com
Re: var-tracking and s390's LM(G)
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. r~
Re: var-tracking and s390's LM(G)
Richard Henderson 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 +
+++ gcc/var-tracking.c 2014-02-06 09:47:57.259364367 +
@@ -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 +
+++ gcc/config/s390/s390.c 2014-02-06 09:48:47.378031557 +
@@ -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
Re: var-tracking and s390's LM(G)
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~
Re: var-tracking and s390's LM(G)
Richard Henderson 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
Re: var-tracking and s390's LM(G)
On 02/06/2014 06:48 AM, Richard Sandiford wrote: > Richard Henderson 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~
