| Message ID | 5338565.7zBeWkNczu@polaris |
|---|---|
| State | New |
| Headers | show |
On Oct 17, 2014, at 1:21 AM, Eric Botcazou <ebotcazou@adacore.com> wrote: > This nevertheless used to work because the blockage insn emitted by the RTL > epilogue was acting as a "wild load" but this got broken by Richard's patch > which removed the "wild load" trick. > The attached patch fixes the breakage by marking the stores as frame related. [ thinking out loud ] So, I can’t help but wonder if c6x, mips, nios2 and sh now have the exact same problem (or could if they switched code-gen some)... > 2014-10-17 Eric Botcazou <ebotcazou@adacore.com> > > * config/arm/arm.c (arm_set_return_address): Mark the store as frame > related, if any. > (thumb_set_return_address): Likewise.
> [ thinking out loud ] So, I can’t help but wonder if c6x, mips, nios2 and sh > now have the exact same problem (or could if they switched code-gen > some)... ARM is very peculiar here since it goes through a stack slot to copy a value into a register; other architectures, e.g. SPARC, do a bare move.
On 17/10/14 09:21, Eric Botcazou wrote: > Hi, > > some OSes, for example VxWorks 6, still use DWARF unwinding on the ARM, which > means that they use __builtin_eh_return (EABI unwinding doesn't). The builtin > is implemented by means of {arm|thumb}_set_return_address, which can generate > a store if LR has been stored on function entry. The problem is that, if this > store is FP-based, it is not seen by the RTL DSE pass as being consumed by the > SP-based load at the same address on function exit. > > That's by design in the RTL DSE pass: FP and SP are never substituted for each > other by cselib, see for example this comment: > > /* The only thing that we are not willing to do (this > is requirement of dse and if others potential uses > need this function we should add a parm to control > it) is that we will not substitute the > STACK_POINTER_REGNUM, FRAME_POINTER or the > HARD_FRAME_POINTER. > > These expansions confuses the code that notices that > stores into the frame go dead at the end of the > function and that the frame is not effected by calls > to subroutines. If you allow the > STACK_POINTER_REGNUM substitution, then dse will > think that parameter pushing also goes dead which is > wrong. If you allow the FRAME_POINTER or the > HARD_FRAME_POINTER then you lose the opportunity to > make the frame assumptions. */ > if (regno == STACK_POINTER_REGNUM > || regno == FRAME_POINTER_REGNUM > || regno == HARD_FRAME_POINTER_REGNUM > || regno == cfa_base_preserved_regno) > return orig; > > so a FP-based store and a SP-based load are never seen as a RAW dependency. > > This nevertheless used to work because the blockage insn emitted by the RTL > epilogue was acting as a "wild load" but this got broken by Richard's patch: > > 2014-03-11 Richard Sandiford <rdsandiford@googlemail.com> > > * builtins.c (expand_builtin_setjmp_receiver): Use and clobber > hard_frame_pointer_rtx. > * cse.c (cse_insn): Remove volatile check. > * cselib.c (cselib_process_insn): Likewise. > * dse.c (scan_insn): Likewise. > > which removed the "wild load" trick. This is visible at -O2 for: > > void > foo (void *c1, void *t1, void *ra) > { > long offset = uw_install_context_1 (c1, t1); > void *handler = __builtin_frob_return_addr (ra); > __builtin_unwind_init (); > __builtin_eh_return (offset, handler); > } > > The attached patch fixes the breakage by marking the stores as frame related. > > Tested on ARM/VxWorks, OK for mainline and 4.9 branch? > > > 2014-10-17 Eric Botcazou <ebotcazou@adacore.com> > > * config/arm/arm.c (arm_set_return_address): Mark the store as frame > related, if any. > (thumb_set_return_address): Likewise. > > OK. R. > > arm_eh_return-2.diff > > > Index: config/arm/arm.c > =================================================================== > --- config/arm/arm.c (revision 216252) > +++ config/arm/arm.c (working copy) > @@ -28952,7 +28952,11 @@ arm_set_return_address (rtx source, rtx > > addr = plus_constant (Pmode, addr, delta); > } > - emit_move_insn (gen_frame_mem (Pmode, addr), source); > + /* The store needs to be marked as frame related in order to prevent > + DSE from deleting it as dead if it is based on fp. */ > + rtx insn = emit_move_insn (gen_frame_mem (Pmode, addr), source); > + RTX_FRAME_RELATED_P (insn) = 1; > + add_reg_note (insn, REG_CFA_RESTORE, gen_rtx_REG (Pmode, LR_REGNUM)); > } > } > > @@ -29004,7 +29008,11 @@ thumb_set_return_address (rtx source, rt > else > addr = plus_constant (Pmode, addr, delta); > > - emit_move_insn (gen_frame_mem (Pmode, addr), source); > + /* The store needs to be marked as frame related in order to prevent > + DSE from deleting it as dead if it is based on fp. */ > + rtx insn = emit_move_insn (gen_frame_mem (Pmode, addr), source); > + RTX_FRAME_RELATED_P (insn) = 1; > + add_reg_note (insn, REG_CFA_RESTORE, gen_rtx_REG (Pmode, LR_REGNUM)); > } > else > emit_move_insn (gen_rtx_REG (Pmode, LR_REGNUM), source); >
Index: config/arm/arm.c =================================================================== --- config/arm/arm.c (revision 216252) +++ config/arm/arm.c (working copy) @@ -28952,7 +28952,11 @@ arm_set_return_address (rtx source, rtx addr = plus_constant (Pmode, addr, delta); } - emit_move_insn (gen_frame_mem (Pmode, addr), source); + /* The store needs to be marked as frame related in order to prevent + DSE from deleting it as dead if it is based on fp. */ + rtx insn = emit_move_insn (gen_frame_mem (Pmode, addr), source); + RTX_FRAME_RELATED_P (insn) = 1; + add_reg_note (insn, REG_CFA_RESTORE, gen_rtx_REG (Pmode, LR_REGNUM)); } } @@ -29004,7 +29008,11 @@ thumb_set_return_address (rtx source, rt else addr = plus_constant (Pmode, addr, delta); - emit_move_insn (gen_frame_mem (Pmode, addr), source); + /* The store needs to be marked as frame related in order to prevent + DSE from deleting it as dead if it is based on fp. */ + rtx insn = emit_move_insn (gen_frame_mem (Pmode, addr), source); + RTX_FRAME_RELATED_P (insn) = 1; + add_reg_note (insn, REG_CFA_RESTORE, gen_rtx_REG (Pmode, LR_REGNUM)); } else emit_move_insn (gen_rtx_REG (Pmode, LR_REGNUM), source);
Hi, some OSes, for example VxWorks 6, still use DWARF unwinding on the ARM, which means that they use __builtin_eh_return (EABI unwinding doesn't). The builtin is implemented by means of {arm|thumb}_set_return_address, which can generate a store if LR has been stored on function entry. The problem is that, if this store is FP-based, it is not seen by the RTL DSE pass as being consumed by the SP-based load at the same address on function exit. That's by design in the RTL DSE pass: FP and SP are never substituted for each other by cselib, see for example this comment: /* The only thing that we are not willing to do (this is requirement of dse and if others potential uses need this function we should add a parm to control it) is that we will not substitute the STACK_POINTER_REGNUM, FRAME_POINTER or the HARD_FRAME_POINTER. These expansions confuses the code that notices that stores into the frame go dead at the end of the function and that the frame is not effected by calls to subroutines. If you allow the STACK_POINTER_REGNUM substitution, then dse will think that parameter pushing also goes dead which is wrong. If you allow the FRAME_POINTER or the HARD_FRAME_POINTER then you lose the opportunity to make the frame assumptions. */ if (regno == STACK_POINTER_REGNUM || regno == FRAME_POINTER_REGNUM || regno == HARD_FRAME_POINTER_REGNUM || regno == cfa_base_preserved_regno) return orig; so a FP-based store and a SP-based load are never seen as a RAW dependency. This nevertheless used to work because the blockage insn emitted by the RTL epilogue was acting as a "wild load" but this got broken by Richard's patch: 2014-03-11 Richard Sandiford <rdsandiford@googlemail.com> * builtins.c (expand_builtin_setjmp_receiver): Use and clobber hard_frame_pointer_rtx. * cse.c (cse_insn): Remove volatile check. * cselib.c (cselib_process_insn): Likewise. * dse.c (scan_insn): Likewise. which removed the "wild load" trick. This is visible at -O2 for: void foo (void *c1, void *t1, void *ra) { long offset = uw_install_context_1 (c1, t1); void *handler = __builtin_frob_return_addr (ra); __builtin_unwind_init (); __builtin_eh_return (offset, handler); } The attached patch fixes the breakage by marking the stores as frame related. Tested on ARM/VxWorks, OK for mainline and 4.9 branch? 2014-10-17 Eric Botcazou <ebotcazou@adacore.com> * config/arm/arm.c (arm_set_return_address): Mark the store as frame related, if any. (thumb_set_return_address): Likewise.