* [PATCH,rs6000] fix interrupt safety issue on E500 targets @ 2007-10-03 14:18 Nathan Froyd 2007-10-03 15:42 ` Andrew Pinski 0 siblings, 1 reply; 13+ messages in thread From: Nathan Froyd @ 2007-10-03 14:18 UTC (permalink / raw) To: gcc-patches [-- Attachment #1: Type: text/plain, Size: 1108 bytes --] The attached patch fixes an interrupt safety problem on E500 targets; this problem was introduced by the ABI fixes that were committed several months ago. On SPE targets we would generate the following code for a function epilogue: point r11 into the stack frame restore registers restore stack pointer return and the instruction scheduler, being clever, would turn this into: point r11 into the stack frame restore a few registers restore stack pointer restore remaining registers (XXX) return Which causes problems if an interrupt happens during XXX because the OS will clobber the space from which we are restoring registers, believing it to be unused. The fix is simple. We just have to tell the scheduler that we have a blocking insn just before the restore of the stack pointer. Tested on powerpc-none-linux-gnuspe with no regressions. OK to commit? -Nathan 2007-10-03 Nathan Froyd <froydnj@codesourcery.com> gcc/ * config/rs6000/rs6000.c (rs6000_emit_epilogue): Emit a stack tie when compiling for SPE targets due to r11 pointing into the frame. [-- Attachment #2: r14-add-insn.patch --] [-- Type: text/x-diff, Size: 999 bytes --] Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 128981) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -16329,13 +16329,18 @@ rs6000_emit_epilogue (int sibcall) } } + if (frame_reg_rtx != sp_reg_rtx + || (TARGET_SPE_ABI + && info->spe_64bit_regs_used != 0 + && info->first_gp_reg_save != 32)) + /* This blockage is needed so that sched doesn't decide to move + the sp change before the register restores. */ + rs6000_emit_stack_tie (); + /* If this is V.4, unwind the stack pointer after all of the loads have been done. */ if (frame_reg_rtx != sp_reg_rtx) { - /* This blockage is needed so that sched doesn't decide to move - the sp change before the register restores. */ - rs6000_emit_stack_tie (); if (TARGET_SPE_ABI && info->spe_64bit_regs_used != 0 && info->first_gp_reg_save != 32) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets 2007-10-03 14:18 [PATCH,rs6000] fix interrupt safety issue on E500 targets Nathan Froyd @ 2007-10-03 15:42 ` Andrew Pinski 2007-10-03 15:45 ` Andrew Pinski 2007-10-04 1:43 ` Alan Modra 0 siblings, 2 replies; 13+ messages in thread From: Andrew Pinski @ 2007-10-03 15:42 UTC (permalink / raw) To: gcc-patches On 10/3/07, Nathan Froyd <froydnj@codesourcery.com> wrote: > Tested on powerpc-none-linux-gnuspe with no regressions. OK to commit? I think this is needed for all sysv based abis and not just SPE. Thanks, Andrew Pinski ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets 2007-10-03 15:42 ` Andrew Pinski @ 2007-10-03 15:45 ` Andrew Pinski 2007-10-03 19:12 ` Nathan Froyd 2007-10-04 1:43 ` Alan Modra 1 sibling, 1 reply; 13+ messages in thread From: Andrew Pinski @ 2007-10-03 15:45 UTC (permalink / raw) To: gcc-patches, froydnj On 10/3/07, Andrew Pinski <pinskia@gmail.com> wrote: > On 10/3/07, Nathan Froyd <froydnj@codesourcery.com> wrote: > > Tested on powerpc-none-linux-gnuspe with no regressions. OK to commit? > > I think this is needed for all sysv based abis and not just SPE. And I think this is related to PR30282. -- Pinski ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets 2007-10-03 15:45 ` Andrew Pinski @ 2007-10-03 19:12 ` Nathan Froyd 2007-10-03 19:17 ` Daniel Jacobowitz 2007-10-03 19:17 ` Andrew Pinski 0 siblings, 2 replies; 13+ messages in thread From: Nathan Froyd @ 2007-10-03 19:12 UTC (permalink / raw) To: Andrew Pinski; +Cc: gcc-patches On Wed, Oct 03, 2007 at 08:45:24AM -0700, Andrew Pinski wrote: > On 10/3/07, Andrew Pinski <pinskia@gmail.com> wrote: > > On 10/3/07, Nathan Froyd <froydnj@codesourcery.com> wrote: > > > Tested on powerpc-none-linux-gnuspe with no regressions. OK to commit? > > > > I think this is needed for all sysv based abis and not just SPE. > > And I think this is related to PR30282. Any objection to simply calling rs6000_emit_stack_tie unconditionally before the stack adjustment, then? (Patch below--I don't know what AIX/Darwin requires here, but I can't imagine it would hurt performance that much.) -Nathan Index: rs6000.c =================================================================== --- rs6000.c (revision 128981) +++ rs6000.c (working copy) @@ -16329,13 +16329,14 @@ rs6000_emit_epilogue (int sibcall) } } + /* This blockage is needed so that sched doesn't decide to move + the sp change before the register restores. */ + rs6000_emit_stack_tie (); + /* If this is V.4, unwind the stack pointer after all of the loads have been done. */ if (frame_reg_rtx != sp_reg_rtx) { - /* This blockage is needed so that sched doesn't decide to move - the sp change before the register restores. */ - rs6000_emit_stack_tie (); if (TARGET_SPE_ABI && info->spe_64bit_regs_used != 0 && info->first_gp_reg_save != 32) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets 2007-10-03 19:12 ` Nathan Froyd @ 2007-10-03 19:17 ` Daniel Jacobowitz 2007-10-03 19:17 ` Andrew Pinski 1 sibling, 0 replies; 13+ messages in thread From: Daniel Jacobowitz @ 2007-10-03 19:17 UTC (permalink / raw) To: Andrew Pinski, gcc-patches On Wed, Oct 03, 2007 at 12:12:06PM -0700, Nathan Froyd wrote: > On Wed, Oct 03, 2007 at 08:45:24AM -0700, Andrew Pinski wrote: > > On 10/3/07, Andrew Pinski <pinskia@gmail.com> wrote: > > > On 10/3/07, Nathan Froyd <froydnj@codesourcery.com> wrote: > > > > Tested on powerpc-none-linux-gnuspe with no regressions. OK to commit? > > > > > > I think this is needed for all sysv based abis and not just SPE. > > > > And I think this is related to PR30282. > > Any objection to simply calling rs6000_emit_stack_tie unconditionally > before the stack adjustment, then? (Patch below--I don't know what > AIX/Darwin requires here, but I can't imagine it would hurt performance > that much.) Do some of these have a red zone? If so, they don't need it. I think PPC64 Linux does. -- Daniel Jacobowitz CodeSourcery ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets 2007-10-03 19:12 ` Nathan Froyd 2007-10-03 19:17 ` Daniel Jacobowitz @ 2007-10-03 19:17 ` Andrew Pinski 2007-10-03 20:10 ` Nathan Froyd 1 sibling, 1 reply; 13+ messages in thread From: Andrew Pinski @ 2007-10-03 19:17 UTC (permalink / raw) To: froydnj, gcc-patches On 10/3/07, Nathan Froyd <froydnj@codesourcery.com> wrote: > Any objection to simply calling rs6000_emit_stack_tie unconditionally > before the stack adjustment, then? (Patch below--I don't know what > AIX/Darwin requires here, but I can't imagine it would hurt performance > that much.) Inside rs6000_stack_info, we have the following comment: /* Determine if we need to allocate any stack frame: For AIX we need to push the stack if a frame pointer is needed (because the stack might be dynamically adjusted), if we are debugging, if we make calls, or if the sum of fp_save, gp_save, and local variables are more than the space needed to save all non-volatile registers: 32-bit: 18*8 + 19*4 = 220 or 64-bit: 18*8 + 18*8 = 288 (GPR13 reserved). For V.4 we don't have the stack cushion that AIX uses, but assume that the debugger can handle stackless frames. */ So only SVS V.4 ABI have this requirement which means PPC64-linux, darwin or AIX have a red zone. And yes this can worse code for OS's with a red zone as the main reason why the movement is happening is so the processor can handle the loads better. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets 2007-10-03 19:17 ` Andrew Pinski @ 2007-10-03 20:10 ` Nathan Froyd 0 siblings, 0 replies; 13+ messages in thread From: Nathan Froyd @ 2007-10-03 20:10 UTC (permalink / raw) To: Andrew Pinski; +Cc: gcc-patches On Wed, Oct 03, 2007 at 12:17:52PM -0700, Andrew Pinski wrote: > On 10/3/07, Nathan Froyd <froydnj@codesourcery.com> wrote: > > Any objection to simply calling rs6000_emit_stack_tie unconditionally > > before the stack adjustment, then? (Patch below--I don't know what > > AIX/Darwin requires here, but I can't imagine it would hurt performance > > that much.) > Inside rs6000_stack_info, we have the following comment: > > So only SVS V.4 ABI have this requirement which means PPC64-linux, > darwin or AIX have a red zone. And yes this can worse code for OS's > with a red zone as the main reason why the movement is happening is so > the processor can handle the loads better. OK, thanks for the explanation. So, modified patch below. -Nathan Index: rs6000.c =================================================================== --- rs6000.c (revision 128981) +++ rs6000.c (working copy) @@ -16329,13 +16329,15 @@ rs6000_emit_epilogue (int sibcall) } } + /* This blockage is needed so that sched doesn't decide to move + the sp change before the register restores. */ + if (DEFAULT_ABI == ABI_V4) + rs6000_emit_stack_tie (); + /* If this is V.4, unwind the stack pointer after all of the loads have been done. */ if (frame_reg_rtx != sp_reg_rtx) { - /* This blockage is needed so that sched doesn't decide to move - the sp change before the register restores. */ - rs6000_emit_stack_tie (); if (TARGET_SPE_ABI && info->spe_64bit_regs_used != 0 && info->first_gp_reg_save != 32) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets 2007-10-03 15:42 ` Andrew Pinski 2007-10-03 15:45 ` Andrew Pinski @ 2007-10-04 1:43 ` Alan Modra 2007-10-05 8:30 ` Alan Modra 1 sibling, 1 reply; 13+ messages in thread From: Alan Modra @ 2007-10-04 1:43 UTC (permalink / raw) To: Andrew Pinski; +Cc: gcc-patches On Wed, Oct 03, 2007 at 08:41:58AM -0700, Andrew Pinski wrote: > On 10/3/07, Nathan Froyd <froydnj@codesourcery.com> wrote: > > Tested on powerpc-none-linux-gnuspe with no regressions. OK to commit? > > I think this is needed for all sysv based abis and not just SPE. No, it shouldn't be needed. Plain powerpc-linux will restore regs using either sp or fp relative memory addressing. gcc shouldn't move the sp adjust above use of sp, which is why we currently only emit the stack tie when frame_reg_rtx != sp_reg_rtx. The problem appears to be specific to spe, which uses yet another register rtx to restore regs. -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets 2007-10-04 1:43 ` Alan Modra @ 2007-10-05 8:30 ` Alan Modra 2007-10-05 15:02 ` Nathan Froyd ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Alan Modra @ 2007-10-05 8:30 UTC (permalink / raw) To: gcc-patches On Thu, Oct 04, 2007 at 11:12:53AM +0930, Alan Modra wrote: > On Wed, Oct 03, 2007 at 08:41:58AM -0700, Andrew Pinski wrote: > > I think this is needed for all sysv based abis and not just SPE. > > No, it shouldn't be needed. Plain powerpc-linux will restore regs > using either sp or fp relative memory addressing. gcc shouldn't move > the sp adjust above use of sp, which is why we currently only emit the > stack tie when frame_reg_rtx != sp_reg_rtx. The problem appears to be > specific to spe, which uses yet another register rtx to restore regs. That wasn't a really clear explanation. What I should have said was: The scheduler will not move the stack adjustment (via sp_reg_rtx) past register loads using a MEM with sp_reg_rtx in the address. We only need the stack tie when we reference MEMs without sp_reg_rtx in their addresses. If you examine rs6000_emit_epilogue you'll see that all of the register restores use frame_reg_rtx in the MEM address, except for some SPE code. frame_reg_rtx is set to sp_reg_rtx at the start of this function, and changed in only one place, for ABI_V4. Thus, prior to the addition of SPE support, it was correct to test frame_reg_rtx != sp_reg_rtx to determine whether a stack tie was needed. So, another fix for the SPE problem is to use frame_reg_rtx in the SPE support code instead of inventing another rtx for r11. That's what I set out to do in the following patch, but then noticed a few other problems.. Firstly, there's a glaring error dating back to when Eric moved the altivec register restore before the stack update. Before we pop the stack frame, sp_offset can't possibly depend on the size of the stack frame, only on whether we have a frame or not. Secondly, the SPE stack restore is wrong when use_backchain_to_restore_sp and spe_regs_addressable_via_sp. In that case, r11 is not set by the SPE register restore code so remains pointing to the top of the stack frame. * config/rs6000/rs6000.c (rs6000_emit_epilogue): Correct altivec sp_offset. Rearrange sp_offset assignments to correspond to stack adjustments. Use frame_reg_rtx for SPE register restores. Correct SPE stack adjustment. A powerpc-linux bootstrap/regtest is still in progress. I don't have access to SPE hardware to properly test the SPE changes, so would someone mind regression testing this for me on SPE? Index: gcc/config/rs6000/rs6000.c =================================================================== --- gcc/config/rs6000/rs6000.c (revision 128777) +++ gcc/config/rs6000/rs6000.c (working copy) @@ -15998,8 +16014,8 @@ rs6000_emit_epilogue (int sibcall) return; } - /* Set sp_offset based on the stack push from the prologue. */ - if (info->total_size < 32767) + /* frame_reg_rtx + sp_offset points to the top of this stack frame. */ + if (info->push_p) sp_offset = info->total_size; /* Restore AltiVec registers if needed. */ @@ -16041,8 +16057,6 @@ rs6000_emit_epilogue (int sibcall) emit_insn (generate_set_vrsave (reg, info, 1)); } - sp_offset = 0; - /* If we have a frame pointer, a call to alloca, or a large stack frame, restore the old stack pointer using the backchain. Otherwise, we know what size to update it with. */ @@ -16055,20 +16069,18 @@ rs6000_emit_epilogue (int sibcall) emit_move_insn (frame_reg_rtx, gen_rtx_MEM (Pmode, sp_reg_rtx)); + sp_offset = 0; } - else if (info->push_p) + else if (info->push_p + && DEFAULT_ABI != ABI_V4 + && !current_function_calls_eh_return) { - if (DEFAULT_ABI == ABI_V4 - || current_function_calls_eh_return) - sp_offset = info->total_size; - else - { - emit_insn (TARGET_32BIT - ? gen_addsi3 (sp_reg_rtx, sp_reg_rtx, - GEN_INT (info->total_size)) - : gen_adddi3 (sp_reg_rtx, sp_reg_rtx, - GEN_INT (info->total_size))); - } + emit_insn (TARGET_32BIT + ? gen_addsi3 (sp_reg_rtx, sp_reg_rtx, + GEN_INT (info->total_size)) + : gen_adddi3 (sp_reg_rtx, sp_reg_rtx, + GEN_INT (info->total_size))); + sp_offset = 0; } /* Get the old lr if we saved it. */ @@ -16150,7 +16162,6 @@ rs6000_emit_epilogue (int sibcall) && info->spe_64bit_regs_used != 0 && info->first_gp_reg_save != 32) { - rtx spe_save_area_ptr; /* Determine whether we can address all of the registers that need to be saved with an offset from the stack pointer that fits in the small const field for SPE memory instructions. */ @@ -16160,20 +16171,21 @@ rs6000_emit_epilogue (int sibcall) int spe_offset; if (spe_regs_addressable_via_sp) - { - spe_save_area_ptr = frame_reg_rtx; - spe_offset = info->spe_gp_save_offset + sp_offset; - } + spe_offset = info->spe_gp_save_offset + sp_offset; else { + rtx old_frame_reg_rtx = frame_reg_rtx; /* Make r11 point to the start of the SPE save area. We worried about not clobbering it when we were saving registers in the prologue. There's no need to worry here because the static chain is passed anew to every function. */ - spe_save_area_ptr = gen_rtx_REG (Pmode, 11); - - emit_insn (gen_addsi3 (spe_save_area_ptr, frame_reg_rtx, + if (frame_reg_rtx == sp_reg_rtx) + frame_reg_rtx = gen_rtx_REG (Pmode, 11); + emit_insn (gen_addsi3 (frame_reg_rtx, old_frame_reg_rtx, GEN_INT (info->spe_gp_save_offset + sp_offset))); + /* Keep the invariant that frame_reg_rtx + sp_offset points + at the top of the stack frame. */ + sp_offset = -info->spe_gp_save_offset; spe_offset = 0; } @@ -16188,7 +16200,7 @@ rs6000_emit_epilogue (int sibcall) gcc_assert (SPE_CONST_OFFSET_OK (spe_offset + reg_size * i)); offset = GEN_INT (spe_offset + reg_size * i); - addr = gen_rtx_PLUS (Pmode, spe_save_area_ptr, offset); + addr = gen_rtx_PLUS (Pmode, frame_reg_rtx, offset); mem = gen_rtx_MEM (V2SImode, addr); emit_move_insn (gen_rtx_REG (reg_mode, info->first_gp_reg_save + i), @@ -16280,11 +16292,9 @@ rs6000_emit_epilogue (int sibcall) /* This blockage is needed so that sched doesn't decide to move the sp change before the register restores. */ rs6000_emit_stack_tie (); - if (TARGET_SPE_ABI - && info->spe_64bit_regs_used != 0 - && info->first_gp_reg_save != 32) - emit_insn (gen_addsi3 (sp_reg_rtx, gen_rtx_REG (Pmode, 11), - GEN_INT (-(info->spe_gp_save_offset + sp_offset)))); + if (sp_offset != 0) + emit_insn (gen_addsi3 (sp_reg_rtx, frame_reg_rtx, + GEN_INT (sp_offset))); else emit_move_insn (sp_reg_rtx, frame_reg_rtx); } -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets 2007-10-05 8:30 ` Alan Modra @ 2007-10-05 15:02 ` Nathan Froyd 2007-10-06 1:01 ` Nathan Froyd 2007-10-16 9:01 ` Alan Modra 2007-10-16 20:31 ` David Edelsohn 2 siblings, 1 reply; 13+ messages in thread From: Nathan Froyd @ 2007-10-05 15:02 UTC (permalink / raw) To: gcc-patches On Fri, Oct 05, 2007 at 06:00:44PM +0930, Alan Modra wrote: > So, another fix for the SPE problem is to use frame_reg_rtx in the SPE > support code instead of inventing another rtx for r11. That's what I > set out to do in the following patch, but then noticed a few other > problems.. > > * config/rs6000/rs6000.c (rs6000_emit_epilogue): Correct > altivec sp_offset. Rearrange sp_offset assignments to > correspond to stack adjustments. Use frame_reg_rtx for > SPE register restores. Correct SPE stack adjustment. > > A powerpc-linux bootstrap/regtest is still in progress. I don't have > access to SPE hardware to properly test the SPE changes, so would > someone mind regression testing this for me on SPE? Bootstrap has proceeded without problems, regtesting underway. -Nathan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets 2007-10-05 15:02 ` Nathan Froyd @ 2007-10-06 1:01 ` Nathan Froyd 0 siblings, 0 replies; 13+ messages in thread From: Nathan Froyd @ 2007-10-06 1:01 UTC (permalink / raw) To: gcc-patches; +Cc: amodra On Fri, Oct 05, 2007 at 08:02:31AM -0700, Nathan Froyd wrote: > On Fri, Oct 05, 2007 at 06:00:44PM +0930, Alan Modra wrote: > > A powerpc-linux bootstrap/regtest is still in progress. I don't have > > access to SPE hardware to properly test the SPE changes, so would > > someone mind regression testing this for me on SPE? > > Bootstrap has proceeded without problems, regtesting underway. Regtesting complete, no regressions. -Nathan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets 2007-10-05 8:30 ` Alan Modra 2007-10-05 15:02 ` Nathan Froyd @ 2007-10-16 9:01 ` Alan Modra 2007-10-16 20:31 ` David Edelsohn 2 siblings, 0 replies; 13+ messages in thread From: Alan Modra @ 2007-10-16 9:01 UTC (permalink / raw) To: gcc-patches On Fri, Oct 05, 2007 at 06:00:44PM +0930, Alan Modra wrote: > * config/rs6000/rs6000.c (rs6000_emit_epilogue): Correct > altivec sp_offset. Rearrange sp_offset assignments to > correspond to stack adjustments. Use frame_reg_rtx for > SPE register restores. Correct SPE stack adjustment. Ping? http://gcc.gnu.org/ml/gcc-patches/2007-10/msg00283.html Besides the SPE issue, the patch also fixes restore of altivec regs in large stack frames for !WORLD_SAVE_P ABIs. Bootstrap and regression testing has of course completed on powerpc-linux. -- Alan Modra Australia Development Lab, IBM ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH,rs6000] fix interrupt safety issue on E500 targets 2007-10-05 8:30 ` Alan Modra 2007-10-05 15:02 ` Nathan Froyd 2007-10-16 9:01 ` Alan Modra @ 2007-10-16 20:31 ` David Edelsohn 2 siblings, 0 replies; 13+ messages in thread From: David Edelsohn @ 2007-10-16 20:31 UTC (permalink / raw) To: Alan Modra; +Cc: gcc-patches * config/rs6000/rs6000.c (rs6000_emit_epilogue): Correct altivec sp_offset. Rearrange sp_offset assignments to correspond to stack adjustments. Use frame_reg_rtx for SPE register restores. Correct SPE stack adjustment. Okay. Thanks, David ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2007-10-16 19:27 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-10-03 14:18 [PATCH,rs6000] fix interrupt safety issue on E500 targets Nathan Froyd 2007-10-03 15:42 ` Andrew Pinski 2007-10-03 15:45 ` Andrew Pinski 2007-10-03 19:12 ` Nathan Froyd 2007-10-03 19:17 ` Daniel Jacobowitz 2007-10-03 19:17 ` Andrew Pinski 2007-10-03 20:10 ` Nathan Froyd 2007-10-04 1:43 ` Alan Modra 2007-10-05 8:30 ` Alan Modra 2007-10-05 15:02 ` Nathan Froyd 2007-10-06 1:01 ` Nathan Froyd 2007-10-16 9:01 ` Alan Modra 2007-10-16 20:31 ` David Edelsohn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).