| Message ID | 4bb05a55fabeab815c2c0224609e0a4f5b68693e.1479209415.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On Tue, Nov 15, 2016 at 6:48 AM, Segher Boessenkool <segher@kernel.crashing.org> wrote: > If we use ABI_V4 and we have a big stack frame, we end the epilogue > with a "mr 1,11" (or similar) instruction. This instruction however > has no dependencies on the earlier restores from stack (done via r11), > so sched2 can end up reordering the insns, which is bad because we > have no red zone so that you then restore from stack that is already > deallocated. > > This fixes it by making that restore depend on the memory accesses. > > Tested on powerpc64-linux {-m32,-m64}; is this okay for trunk? Do we > want a testcase for this? > > > Segher > > > 2016-11-15 Segher Boessenkool <segher@kernel.crashing.org> > > * config/rs6000/rs6000.c (rs6000_emit_stack_reset): Emit the > stack_restore_tie insn instead of stack_tie, for the SVR4 and > SPE ABIs. > * config/rs6000/rs6000.md (stack_restore_tie): New define_insn. Okay. A similar change may be necessary for other uses of rs6000_stack_tie in the prologue. It would be good to comment somewhere that stack_restore_tie is a superset of rs6000_stack_tie. Thanks, David
On Tue, Nov 15, 2016 at 09:06:23AM -0500, David Edelsohn wrote: > > * config/rs6000/rs6000.c (rs6000_emit_stack_reset): Emit the > > stack_restore_tie insn instead of stack_tie, for the SVR4 and > > SPE ABIs. > > * config/rs6000/rs6000.md (stack_restore_tie): New define_insn. > > Okay. > > A similar change may be necessary for other uses of rs6000_stack_tie > in the prologue. I thought just a little bit more staring at the prologue code would let me handle this, but so far no luck. So, I'll commit the original patch now, some progress is better than no progress. Segher
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 8a04248..2ceddfd 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -27487,7 +27487,11 @@ rs6000_emit_stack_reset (rs6000_stack_t *info, rtx frame_reg_rtx, HOST_WIDE_INT frame_off, unsigned updt_regno) { - rtx updt_reg_rtx; + /* If there is nothing to do, don't do anything. */ + if (frame_off == 0 && REGNO (frame_reg_rtx) == updt_regno) + return NULL_RTX; + + rtx updt_reg_rtx = gen_rtx_REG (Pmode, updt_regno); /* This blockage is needed so that sched doesn't decide to move the sp change before the register restores. */ @@ -27495,18 +27499,17 @@ rs6000_emit_stack_reset (rs6000_stack_t *info, || (TARGET_SPE_ABI && info->spe_64bit_regs_used != 0 && info->first_gp_reg_save != 32)) - rs6000_emit_stack_tie (frame_reg_rtx, frame_pointer_needed); + return emit_insn (gen_stack_restore_tie (updt_reg_rtx, frame_reg_rtx, + GEN_INT (frame_off))); /* If we are restoring registers out-of-line, we will be using the "exit" variants of the restore routines, which will reset the stack for us. But we do need to point updt_reg into the right place for those routines. */ - updt_reg_rtx = gen_rtx_REG (Pmode, updt_regno); - if (frame_off != 0) return emit_insn (gen_add3_insn (updt_reg_rtx, frame_reg_rtx, GEN_INT (frame_off))); - else if (REGNO (frame_reg_rtx) != updt_regno) + else return emit_move_insn (updt_reg_rtx, frame_reg_rtx); return NULL_RTX; diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md index b3fe92a..a779f5c 100644 --- a/gcc/config/rs6000/rs6000.md +++ b/gcc/config/rs6000/rs6000.md @@ -12769,6 +12769,22 @@ (define_insn "stack_tie" "" [(set_attr "length" "0")]) +; Some 32-bit ABIs do not have a red zone, so the stack deallocation has to +; stay behind all restores from the stack, it cannot be reordered to before +; one. See PR77687. +(define_insn "stack_restore_tie" + [(set (match_operand:SI 0 "gpc_reg_operand" "=r,r") + (plus:SI (match_operand:SI 1 "gpc_reg_operand" "r,r") + (match_operand:SI 2 "reg_or_cint_operand" "O,rI"))) + (set (mem:BLK (match_dup 0)) (const_int 0)) + (set (mem:BLK (match_dup 1)) (const_int 0))] +; (clobber (mem:BLK (scratch:SI)))] + "TARGET_32BIT" + "@ + mr %0,%1 + add%I2 %0,%1,%2" + [(set_attr "type" "*,add")]) + (define_expand "epilogue" [(use (const_int 0))] ""
If we use ABI_V4 and we have a big stack frame, we end the epilogue with a "mr 1,11" (or similar) instruction. This instruction however has no dependencies on the earlier restores from stack (done via r11), so sched2 can end up reordering the insns, which is bad because we have no red zone so that you then restore from stack that is already deallocated. This fixes it by making that restore depend on the memory accesses. Tested on powerpc64-linux {-m32,-m64}; is this okay for trunk? Do we want a testcase for this? Segher 2016-11-15 Segher Boessenkool <segher@kernel.crashing.org> * config/rs6000/rs6000.c (rs6000_emit_stack_reset): Emit the stack_restore_tie insn instead of stack_tie, for the SVR4 and SPE ABIs. * config/rs6000/rs6000.md (stack_restore_tie): New define_insn. --- gcc/config/rs6000/rs6000.c | 13 ++++++++----- gcc/config/rs6000/rs6000.md | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 5 deletions(-)