| Message ID | 4a2628fc-5e4a-1bb1-e0f7-88fc26712bfd@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] rs6000: Don't use HARD_FRAME_POINTER_REGNUM if it's not live in pro_and_epilogue (PR91518) | expand |
Hi! On Mon, Apr 13, 2020 at 10:11:43AM +0800, luoxhu wrote: > frame_pointer_needed is set to true in reload pass setup_can_eliminate, > but regs_ever_live[31] is false, pro_and_epilogue uses it without live > check causing CPU2006 465.tonto segment fault of loading from invalid > addresses due to r31 not saved/restored. Thus, add HARD_FRAME_POINTER_REGNUM > live check with frame_pointer_needed_indeed_p when generating pro_and_epilogue > instructions. I see. Can you instead make a boolean variable "frame_pointer_needed_indeed", that you set somewhere early in *logue processing? So that we can be sure that it will not change behind our backs. > void > rs6000_emit_prologue_components (sbitmap components) > { > rs6000_stack_t *info = rs6000_stack_info (); > - rtx ptr_reg = gen_rtx_REG (Pmode, frame_pointer_needed > - ? HARD_FRAME_POINTER_REGNUM > - : STACK_POINTER_REGNUM); > + rtx ptr_reg = gen_rtx_REG (Pmode, frame_pointer_needed_indeed_p () > + ? HARD_FRAME_POINTER_REGNUM > + : STACK_POINTER_REGNUM); Yeah, I got the indent wrong there, thanks for fixing it :-) These four cases might well be the only four you need to fix here, but I'll double-check it tomorrow, when I'm awake ;-) Thanks! Segher
On 2020/4/17 08:52, Segher Boessenkool wrote: > Hi! > > On Mon, Apr 13, 2020 at 10:11:43AM +0800, luoxhu wrote: >> frame_pointer_needed is set to true in reload pass setup_can_eliminate, >> but regs_ever_live[31] is false, pro_and_epilogue uses it without live >> check causing CPU2006 465.tonto segment fault of loading from invalid >> addresses due to r31 not saved/restored. Thus, add HARD_FRAME_POINTER_REGNUM >> live check with frame_pointer_needed_indeed_p when generating pro_and_epilogue >> instructions. > > I see. > > Can you instead make a boolean variable "frame_pointer_needed_indeed", > that you set somewhere early in *logue processing? So that we can be > sure that it will not change behind our backs. Thanks, rs6000_emit_prologue seems the proper place to set the frame_pointer_needed_indeed, but it's strange that hard_frame_pointer_rtx will be marked USE in make_prologue_seq, also need check here though not causing segfault? PS, this piece of code is in different file. function.c static rtx_insn * make_prologue_seq (void) { if (!targetm.have_prologue ()) return NULL; start_sequence (); rtx_insn *seq = targetm.gen_prologue (); emit_insn (seq); /* Insert an explicit USE for the frame pointer if the profiling is on and the frame pointer is required. */ if (crtl->profile && frame_pointer_needed) emit_use (hard_frame_pointer_rtx); ... Any way, update the patch as below with your previous comments: This bug is exposed by FRE refactor of r263875. Comparing the fre dump file shows no obvious change of the segment fault function proves it to be a target issue. frame_pointer_needed is set to true in reload pass setup_can_eliminate, but regs_ever_live[31] is false, pro_and_epilogue uses it without live check causing CPU2006 465.tonto segment fault of loading from invalid addresses due to r31 not saved/restored. Thus, add HARD_FRAME_POINTER_REGNUM live check with frame_pointer_needed_indeed when generating pro_and_epilogue instructions. Bootstrap and regression tested pass on Power8-LE. Backport to gcc-9 required once approved. gcc/ChangeLog 2020-04-17 Xiong Hu Luo <luoxhu@linux.ibm.com> PR target/91518 * config/rs6000/rs6000-logue.c (frame_pointer_needed_indeed): New variable. (rs6000_emit_prologue_components): Check with frame_pointer_needed_indeed. (rs6000_emit_epilogue_components): Likewise. (rs6000_emit_epilogue): Likewise. (rs6000_emit_prologue): Set frame_pointer_needed_indeed. --- gcc/config/rs6000/rs6000-logue.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c index 4cbf228eb79..2213d1fa227 100644 --- a/gcc/config/rs6000/rs6000-logue.c +++ b/gcc/config/rs6000/rs6000-logue.c @@ -58,6 +58,8 @@ static bool rs6000_save_toc_in_prologue_p (void); static rs6000_stack_t stack_info; +/* Set if HARD_FRAM_POINTER_REGNUM is really needed. */ +static bool frame_pointer_needed_indeed = false; /* Label number of label created for -mrelocatable, to call to so we can get the address of the GOT section */ @@ -2735,9 +2737,9 @@ void rs6000_emit_prologue_components (sbitmap components) { rs6000_stack_t *info = rs6000_stack_info (); - rtx ptr_reg = gen_rtx_REG (Pmode, frame_pointer_needed - ? HARD_FRAME_POINTER_REGNUM - : STACK_POINTER_REGNUM); + rtx ptr_reg = gen_rtx_REG (Pmode, frame_pointer_needed_indeed + ? HARD_FRAME_POINTER_REGNUM + : STACK_POINTER_REGNUM); machine_mode reg_mode = Pmode; int reg_size = TARGET_32BIT ? 4 : 8; @@ -2815,9 +2817,9 @@ void rs6000_emit_epilogue_components (sbitmap components) { rs6000_stack_t *info = rs6000_stack_info (); - rtx ptr_reg = gen_rtx_REG (Pmode, frame_pointer_needed - ? HARD_FRAME_POINTER_REGNUM - : STACK_POINTER_REGNUM); + rtx ptr_reg = gen_rtx_REG (Pmode, frame_pointer_needed_indeed + ? HARD_FRAME_POINTER_REGNUM + : STACK_POINTER_REGNUM); machine_mode reg_mode = Pmode; int reg_size = TARGET_32BIT ? 4 : 8; @@ -2996,7 +2998,10 @@ rs6000_emit_prologue (void) && (lookup_attribute ("no_split_stack", DECL_ATTRIBUTES (cfun->decl)) == NULL)); - + + frame_pointer_needed_indeed + = frame_pointer_needed && df_regs_ever_live_p (HARD_FRAME_POINTER_REGNUM); + /* Offset to top of frame for frame_reg and sp respectively. */ HOST_WIDE_INT frame_off = 0; HOST_WIDE_INT sp_off = 0; @@ -3658,7 +3663,7 @@ rs6000_emit_prologue (void) } /* Set frame pointer, if needed. */ - if (frame_pointer_needed) + if (frame_pointer_needed_indeed) { insn = emit_move_insn (gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM), sp_reg_rtx); @@ -4534,7 +4539,7 @@ rs6000_emit_epilogue (enum epilogue_type epilogue_type) } /* If we have a frame pointer, we can restore the old stack pointer from it. */ - else if (frame_pointer_needed) + else if (frame_pointer_needed_indeed) { frame_reg_rtx = sp_reg_rtx; if (DEFAULT_ABI == ABI_V4)
Hi! On Fri, Apr 17, 2020 at 01:35:15PM +0800, luoxhu wrote: > On 2020/4/17 08:52, Segher Boessenkool wrote: > > On Mon, Apr 13, 2020 at 10:11:43AM +0800, luoxhu wrote: > >> frame_pointer_needed is set to true in reload pass setup_can_eliminate, > >> but regs_ever_live[31] is false, pro_and_epilogue uses it without live > >> check causing CPU2006 465.tonto segment fault of loading from invalid > >> addresses due to r31 not saved/restored. Thus, add HARD_FRAME_POINTER_REGNUM > >> live check with frame_pointer_needed_indeed_p when generating pro_and_epilogue > >> instructions. > > > > I see. > > > > Can you instead make a boolean variable "frame_pointer_needed_indeed", > > that you set somewhere early in *logue processing? So that we can be > > sure that it will not change behind our backs. > > > Thanks, rs6000_emit_prologue seems the proper place to set the frame_pointer_needed_indeed, Yeah. > but it's strange that hard_frame_pointer_rtx will be marked USE in make_prologue_seq, also This is for when profiling is enabled. If this routine itself does not use the hard frame pointer, the profiling code (not generated by GCC here!) could still use it, so we need the USE to make sure later GCC passes will see the hard frame pointer as live. This is generic code; it isn't relevant for rs6000 I think. > need check here though not causing segfault? I'm not sure how it would? > function.c > static rtx_insn * > make_prologue_seq (void) > { > if (!targetm.have_prologue ()) > return NULL; > > start_sequence (); > rtx_insn *seq = targetm.gen_prologue (); > emit_insn (seq); > > /* Insert an explicit USE for the frame pointer > if the profiling is on and the frame pointer is required. */ > if (crtl->profile && frame_pointer_needed) > emit_use (hard_frame_pointer_rtx); > ... > Any way, update the patch as below with your previous comments: > > > > This bug is exposed by FRE refactor of r263875. Comparing the fre > dump file shows no obvious change of the segment fault function proves > it to be a target issue. > frame_pointer_needed is set to true in reload pass setup_can_eliminate, > but regs_ever_live[31] is false, pro_and_epilogue uses it without live > check causing CPU2006 465.tonto segment fault of loading from invalid > addresses due to r31 not saved/restored. Thus, add HARD_FRAME_POINTER_REGNUM > live check with frame_pointer_needed_indeed when generating pro_and_epilogue > instructions. > > Bootstrap and regression tested pass on Power8-LE. Backport to gcc-9 > required once approved. > > gcc/ChangeLog > > 2020-04-17 Xiong Hu Luo <luoxhu@linux.ibm.com> > > PR target/91518 > * config/rs6000/rs6000-logue.c (frame_pointer_needed_indeed): > New variable. > (rs6000_emit_prologue_components): > Check with frame_pointer_needed_indeed. > (rs6000_emit_epilogue_components): Likewise. > (rs6000_emit_epilogue): Likewise. > (rs6000_emit_prologue): Set frame_pointer_needed_indeed. The patch is okay for trunk. (And for backports later). Thank you! Segher
diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c index 4cbf228eb79..d17876ac0fb 100644 --- a/gcc/config/rs6000/rs6000-logue.c +++ b/gcc/config/rs6000/rs6000-logue.c @@ -2730,14 +2730,22 @@ rs6000_disqualify_components (sbitmap components, edge e, } } +/* Determine whether HARD_FRAM_POINTER_REGNUM is really needed. */ +static bool +frame_pointer_needed_indeed_p (void) +{ + return frame_pointer_needed + && df_regs_ever_live_p (HARD_FRAME_POINTER_REGNUM); +} + /* Implement TARGET_SHRINK_WRAP_EMIT_PROLOGUE_COMPONENTS. */ void rs6000_emit_prologue_components (sbitmap components) { rs6000_stack_t *info = rs6000_stack_info (); - rtx ptr_reg = gen_rtx_REG (Pmode, frame_pointer_needed - ? HARD_FRAME_POINTER_REGNUM - : STACK_POINTER_REGNUM); + rtx ptr_reg = gen_rtx_REG (Pmode, frame_pointer_needed_indeed_p () + ? HARD_FRAME_POINTER_REGNUM + : STACK_POINTER_REGNUM); machine_mode reg_mode = Pmode; int reg_size = TARGET_32BIT ? 4 : 8; @@ -2815,9 +2823,9 @@ void rs6000_emit_epilogue_components (sbitmap components) { rs6000_stack_t *info = rs6000_stack_info (); - rtx ptr_reg = gen_rtx_REG (Pmode, frame_pointer_needed - ? HARD_FRAME_POINTER_REGNUM - : STACK_POINTER_REGNUM); + rtx ptr_reg = gen_rtx_REG (Pmode, frame_pointer_needed_indeed_p () + ? HARD_FRAME_POINTER_REGNUM + : STACK_POINTER_REGNUM); machine_mode reg_mode = Pmode; int reg_size = TARGET_32BIT ? 4 : 8; @@ -3658,7 +3666,7 @@ rs6000_emit_prologue (void) } /* Set frame pointer, if needed. */ - if (frame_pointer_needed) + if (frame_pointer_needed_indeed_p ()) { insn = emit_move_insn (gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM), sp_reg_rtx); @@ -4534,7 +4542,7 @@ rs6000_emit_epilogue (enum epilogue_type epilogue_type) } /* If we have a frame pointer, we can restore the old stack pointer from it. */ - else if (frame_pointer_needed) + else if (frame_pointer_needed_indeed_p ()) { frame_reg_rtx = sp_reg_rtx; if (DEFAULT_ABI == ABI_V4)