| Message ID | 20150518025407.GU6140@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
On Sun, May 17, 2015 at 10:54 PM, Alan Modra <amodra@gmail.com> wrote: > This patch changes rs6000_stack_info to keep save areas offsets even > when not used. I need lr_save_offset valid for split-stack, and it > seemed reasonable to treat the other offsets the same. Not zeroing > the offsets requires just one change in code that uses them, the > use_backchain_to_restore_sp expression in rs6000_emit_epilogue, not > counting the debug_stack_info changes. > > * config/rs6000/rs6000.c (rs6000_stack_info): Don't zero offsets > when not saving registers. > (debug_stack_info): Adjust to omit printing unused offsets, > as before. > (rs6000_emit_epilogue): Adjust use_backchain_to_restore_sp > expression. I think that the vrsave_save_offset change may break saving of callee-saved VRs. See PR 55276. Additional points for converting the testcase into one that can be included in the GCC testsuite. Thanks, David
On Mon, May 18, 2015 at 02:05:59PM -0400, David Edelsohn wrote: > On Sun, May 17, 2015 at 10:54 PM, Alan Modra <amodra@gmail.com> wrote: > > This patch changes rs6000_stack_info to keep save areas offsets even > > when not used. I need lr_save_offset valid for split-stack, and it > > seemed reasonable to treat the other offsets the same. Not zeroing > > the offsets requires just one change in code that uses them, the > > use_backchain_to_restore_sp expression in rs6000_emit_epilogue, not > > counting the debug_stack_info changes. > > > > * config/rs6000/rs6000.c (rs6000_stack_info): Don't zero offsets > > when not saving registers. > > (debug_stack_info): Adjust to omit printing unused offsets, > > as before. > > (rs6000_emit_epilogue): Adjust use_backchain_to_restore_sp > > expression. > > I think that the vrsave_save_offset change may break saving of > callee-saved VRs. See PR 55276. I checked. It doesn't break that testcase. PR 55276 was really caused by using vrsave_mask for two purposes, firstly to track which altivec registers have been saved, and secondly to control use of the vrsave stack slot and whether mfvrsave/mtvrsave insns are generated. Patch 2/4 removes this conflation.
On Tue, May 19, 2015 at 9:09 PM, Alan Modra <amodra@gmail.com> wrote: > On Mon, May 18, 2015 at 02:05:59PM -0400, David Edelsohn wrote: >> On Sun, May 17, 2015 at 10:54 PM, Alan Modra <amodra@gmail.com> wrote: >> > This patch changes rs6000_stack_info to keep save areas offsets even >> > when not used. I need lr_save_offset valid for split-stack, and it >> > seemed reasonable to treat the other offsets the same. Not zeroing >> > the offsets requires just one change in code that uses them, the >> > use_backchain_to_restore_sp expression in rs6000_emit_epilogue, not >> > counting the debug_stack_info changes. >> > >> > * config/rs6000/rs6000.c (rs6000_stack_info): Don't zero offsets >> > when not saving registers. >> > (debug_stack_info): Adjust to omit printing unused offsets, >> > as before. >> > (rs6000_emit_epilogue): Adjust use_backchain_to_restore_sp >> > expression. >> >> I think that the vrsave_save_offset change may break saving of >> callee-saved VRs. See PR 55276. > > I checked. It doesn't break that testcase. PR 55276 was really > caused by using vrsave_mask for two purposes, firstly to track which > altivec registers have been saved, and secondly to control use of the > vrsave stack slot and whether mfvrsave/mtvrsave insns are generated. > Patch 2/4 removes this conflation. Okay, but that confirms Patch 1 is not safe without the patch series. - David
On Wed, May 20, 2015 at 09:02:40AM -0400, David Edelsohn wrote: > On Tue, May 19, 2015 at 9:09 PM, Alan Modra <amodra@gmail.com> wrote: > > On Mon, May 18, 2015 at 02:05:59PM -0400, David Edelsohn wrote: > >> On Sun, May 17, 2015 at 10:54 PM, Alan Modra <amodra@gmail.com> wrote: > >> > This patch changes rs6000_stack_info to keep save areas offsets even > >> > when not used. I need lr_save_offset valid for split-stack, and it > >> > seemed reasonable to treat the other offsets the same. Not zeroing > >> > the offsets requires just one change in code that uses them, the > >> > use_backchain_to_restore_sp expression in rs6000_emit_epilogue, not > >> > counting the debug_stack_info changes. > >> > > >> > * config/rs6000/rs6000.c (rs6000_stack_info): Don't zero offsets > >> > when not saving registers. > >> > (debug_stack_info): Adjust to omit printing unused offsets, > >> > as before. > >> > (rs6000_emit_epilogue): Adjust use_backchain_to_restore_sp > >> > expression. > >> > >> I think that the vrsave_save_offset change may break saving of > >> callee-saved VRs. See PR 55276. > > > > I checked. It doesn't break that testcase. PR 55276 was really > > caused by using vrsave_mask for two purposes, firstly to track which > > altivec registers have been saved, and secondly to control use of the > > vrsave stack slot and whether mfvrsave/mtvrsave insns are generated. > > Patch 2/4 removes this conflation. > > Okay, but that confirms Patch 1 is not safe without the patch series. No, patch 1/4 is safe by itself. That's what I tested when I said I'd checked. Patch 2/4 doesn't correct a fault in patch 1/4. The explanation I gave re PR 55276 is saying that patch 2/4 prevents the confusion that caused PR 55276 from re-occurring, at least as far as vrsave_mask is concerned.
diff -urp gcc-virgin/gcc/config/rs6000/rs6000.c gcc-stack-info1/gcc/config/rs6000/rs6000.c --- gcc-virgin/gcc/config/rs6000/rs6000.c 2015-05-15 14:15:38.157244403 +0930 +++ gcc-stack-info1/gcc/config/rs6000/rs6000.c 2015-05-18 09:44:34.027608414 +0930 @@ -22014,31 +22014,6 @@ rs6000_stack_info (void) else info_ptr->push_p = non_fixed_size > (TARGET_32BIT ? 220 : 288); - /* Zero offsets if we're not saving those registers. */ - if (info_ptr->fp_size == 0) - info_ptr->fp_save_offset = 0; - - if (info_ptr->gp_size == 0) - info_ptr->gp_save_offset = 0; - - if (! TARGET_ALTIVEC_ABI || info_ptr->altivec_size == 0) - info_ptr->altivec_save_offset = 0; - - /* Zero VRSAVE offset if not saved and restored. */ - if (! TARGET_ALTIVEC_VRSAVE || info_ptr->vrsave_mask == 0) - info_ptr->vrsave_save_offset = 0; - - if (! TARGET_SPE_ABI - || info_ptr->spe_64bit_regs_used == 0 - || info_ptr->spe_gp_size == 0) - info_ptr->spe_gp_save_offset = 0; - - if (! info_ptr->lr_save_p) - info_ptr->lr_save_offset = 0; - - if (! info_ptr->cr_save_p) - info_ptr->cr_save_offset = 0; - return info_ptr; } @@ -22144,28 +22119,28 @@ debug_stack_info (rs6000_stack_t *info) if (info->calls_p) fprintf (stderr, "\tcalls_p = %5d\n", info->calls_p); - if (info->gp_save_offset) + if (info->gp_size) fprintf (stderr, "\tgp_save_offset = %5d\n", info->gp_save_offset); - if (info->fp_save_offset) + if (info->fp_size) fprintf (stderr, "\tfp_save_offset = %5d\n", info->fp_save_offset); - if (info->altivec_save_offset) + if (info->altivec_size) fprintf (stderr, "\taltivec_save_offset = %5d\n", info->altivec_save_offset); - if (info->spe_gp_save_offset) + if (info->spe_gp_size == 0) fprintf (stderr, "\tspe_gp_save_offset = %5d\n", info->spe_gp_save_offset); - if (info->vrsave_save_offset) + if (info->vrsave_size) fprintf (stderr, "\tvrsave_save_offset = %5d\n", info->vrsave_save_offset); - if (info->lr_save_offset) + if (info->lr_save_p) fprintf (stderr, "\tlr_save_offset = %5d\n", info->lr_save_offset); - if (info->cr_save_offset) + if (info->cr_save_p) fprintf (stderr, "\tcr_save_offset = %5d\n", info->cr_save_offset); if (info->varargs_save_offset) @@ -24736,7 +24711,9 @@ rs6000_emit_epilogue (int sibcall) here will not trigger at the moment; We don't actually need a frame pointer for alloca, but the generic parts of the compiler give us one anyway. */ - use_backchain_to_restore_sp = (info->total_size > 32767 - info->lr_save_offset + use_backchain_to_restore_sp = (info->total_size + (info->lr_save_p + ? info->lr_save_offset + : 0) > 32767 || (cfun->calls_alloca && !frame_pointer_needed)); restore_lr = (info->lr_save_p