| Message ID | 20190208004350.GD5398@bubble.grove.modra.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RS6000] Correct save_reg_p | expand |
On Fri, Feb 08, 2019 at 11:13:50AM +1030, Alan Modra wrote: > PR target/88343 > * config/rs6000/rs6000.c (save_reg_p): Correct calls_eh_return > case. Match logic in rs6000_emit_prologue emitting pic_offset_table > setup, simplifying ABI_V4 case to df_regs_ever_live_p. That one regressed gcc.dg/20020312-2.c, due to my "cleverness" in simplifying the ABI_V4 case. This one passes regression testing. OK to apply? PR target/88343 * config/rs6000/rs6000.c (save_reg_p): Correct calls_eh_return case. Match logic in rs6000_emit_prologue emitting pic_offset_table setup. diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index cced90bb518..b4908c4f795 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -24008,21 +24008,30 @@ rs6000_split_multireg_move (rtx dst, rtx src) static bool save_reg_p (int reg) { - /* We need to mark the PIC offset register live for the same conditions - as it is set up, or otherwise it won't be saved before we clobber it. */ - if (reg == RS6000_PIC_OFFSET_TABLE_REGNUM && !TARGET_SINGLE_PIC_BASE) { /* When calling eh_return, we must return true for all the cases where conditional_register_usage marks the PIC offset reg - call used. */ + call used or fixed. */ + if (crtl->calls_eh_return + && ((DEFAULT_ABI == ABI_V4 && flag_pic) + || (DEFAULT_ABI == ABI_DARWIN && flag_pic) + || (TARGET_TOC && TARGET_MINIMAL_TOC))) + return true; + + /* We need to mark the PIC offset register live for the same + conditions as it is set up in rs6000_emit_prologue, or + otherwise it won't be saved before we clobber it. */ if (TARGET_TOC && TARGET_MINIMAL_TOC - && (crtl->calls_eh_return - || df_regs_ever_live_p (reg) - || !constant_pool_empty_p ())) + && !constant_pool_empty_p ()) + return true; + + if (DEFAULT_ABI == ABI_V4 + && (flag_pic == 1 || (flag_pic && TARGET_SECURE_PLT)) + && df_regs_ever_live_p (RS6000_PIC_OFFSET_TABLE_REGNUM)) return true; - if ((DEFAULT_ABI == ABI_V4 || DEFAULT_ABI == ABI_DARWIN) + if (DEFAULT_ABI == ABI_DARWIN && flag_pic && crtl->uses_pic_offset_table) return true; }
On Fri, Feb 08, 2019 at 10:19:40PM +1030, Alan Modra wrote: > That one regressed gcc.dg/20020312-2.c, due to my "cleverness" in > simplifying the ABI_V4 case. This one passes regression testing. > OK to apply? I think this is correct. Thanks! Okay for trunk. Does it need backports? Segher > PR target/88343 > * config/rs6000/rs6000.c (save_reg_p): Correct calls_eh_return > case. Match logic in rs6000_emit_prologue emitting pic_offset_table > setup. > > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c > index cced90bb518..b4908c4f795 100644 > --- a/gcc/config/rs6000/rs6000.c > +++ b/gcc/config/rs6000/rs6000.c > @@ -24008,21 +24008,30 @@ rs6000_split_multireg_move (rtx dst, rtx src) > static bool > save_reg_p (int reg) > { > - /* We need to mark the PIC offset register live for the same conditions > - as it is set up, or otherwise it won't be saved before we clobber it. */ > - > if (reg == RS6000_PIC_OFFSET_TABLE_REGNUM && !TARGET_SINGLE_PIC_BASE) > { > /* When calling eh_return, we must return true for all the cases > where conditional_register_usage marks the PIC offset reg > - call used. */ > + call used or fixed. */ > + if (crtl->calls_eh_return > + && ((DEFAULT_ABI == ABI_V4 && flag_pic) > + || (DEFAULT_ABI == ABI_DARWIN && flag_pic) > + || (TARGET_TOC && TARGET_MINIMAL_TOC))) > + return true; > + > + /* We need to mark the PIC offset register live for the same > + conditions as it is set up in rs6000_emit_prologue, or > + otherwise it won't be saved before we clobber it. */ > if (TARGET_TOC && TARGET_MINIMAL_TOC > - && (crtl->calls_eh_return > - || df_regs_ever_live_p (reg) > - || !constant_pool_empty_p ())) > + && !constant_pool_empty_p ()) > + return true; > + > + if (DEFAULT_ABI == ABI_V4 > + && (flag_pic == 1 || (flag_pic && TARGET_SECURE_PLT)) > + && df_regs_ever_live_p (RS6000_PIC_OFFSET_TABLE_REGNUM)) > return true; > > - if ((DEFAULT_ABI == ABI_V4 || DEFAULT_ABI == ABI_DARWIN) > + if (DEFAULT_ABI == ABI_DARWIN > && flag_pic && crtl->uses_pic_offset_table) > return true; > }
> On 8 Feb 2019, at 16:16, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Fri, Feb 08, 2019 at 10:19:40PM +1030, Alan Modra wrote: >> That one regressed gcc.dg/20020312-2.c, due to my "cleverness" in >> simplifying the ABI_V4 case. This one passes regression testing. >> OK to apply? > > I think this is correct. Thanks! Okay for trunk. Does it need backports? Yes, we reverted the original fix on 7 and 8. Iain > > > Segher > > >> PR target/88343 >> * config/rs6000/rs6000.c (save_reg_p): Correct calls_eh_return >> case. Match logic in rs6000_emit_prologue emitting pic_offset_table >> setup. >> >> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c >> index cced90bb518..b4908c4f795 100644 >> --- a/gcc/config/rs6000/rs6000.c >> +++ b/gcc/config/rs6000/rs6000.c >> @@ -24008,21 +24008,30 @@ rs6000_split_multireg_move (rtx dst, rtx src) >> static bool >> save_reg_p (int reg) >> { >> - /* We need to mark the PIC offset register live for the same conditions >> - as it is set up, or otherwise it won't be saved before we clobber it. */ >> - >> if (reg == RS6000_PIC_OFFSET_TABLE_REGNUM && !TARGET_SINGLE_PIC_BASE) >> { >> /* When calling eh_return, we must return true for all the cases >> where conditional_register_usage marks the PIC offset reg >> - call used. */ >> + call used or fixed. */ >> + if (crtl->calls_eh_return >> + && ((DEFAULT_ABI == ABI_V4 && flag_pic) >> + || (DEFAULT_ABI == ABI_DARWIN && flag_pic) >> + || (TARGET_TOC && TARGET_MINIMAL_TOC))) >> + return true; >> + >> + /* We need to mark the PIC offset register live for the same >> + conditions as it is set up in rs6000_emit_prologue, or >> + otherwise it won't be saved before we clobber it. */ >> if (TARGET_TOC && TARGET_MINIMAL_TOC >> - && (crtl->calls_eh_return >> - || df_regs_ever_live_p (reg) >> - || !constant_pool_empty_p ())) >> + && !constant_pool_empty_p ()) >> + return true; >> + >> + if (DEFAULT_ABI == ABI_V4 >> + && (flag_pic == 1 || (flag_pic && TARGET_SECURE_PLT)) >> + && df_regs_ever_live_p (RS6000_PIC_OFFSET_TABLE_REGNUM)) >> return true; >> >> - if ((DEFAULT_ABI == ABI_V4 || DEFAULT_ABI == ABI_DARWIN) >> + if (DEFAULT_ABI == ABI_DARWIN >> && flag_pic && crtl->uses_pic_offset_table) >> return true; >> }
On Fri, Feb 08, 2019 at 04:18:52PM +0000, Iain Sandoe wrote: > > > On 8 Feb 2019, at 16:16, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > > > On Fri, Feb 08, 2019 at 10:19:40PM +1030, Alan Modra wrote: > >> That one regressed gcc.dg/20020312-2.c, due to my "cleverness" in > >> simplifying the ABI_V4 case. This one passes regression testing. > >> OK to apply? > > > > I think this is correct. Thanks! Okay for trunk. Does it need backports? > > Yes, we reverted the original fix on 7 and 8. Then I hope we can make a working 8.3 :-/ Alan, to increase the chances of that, please backport it to 8 as soon as you are confident trunk works? Have tested it, I mean :-) Segher
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index cced90bb518..9933d4443f7 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -24013,18 +24013,26 @@ save_reg_p (int reg) if (reg == RS6000_PIC_OFFSET_TABLE_REGNUM && !TARGET_SINGLE_PIC_BASE) { + if (df_regs_ever_live_p (reg)) + return true; + /* When calling eh_return, we must return true for all the cases where conditional_register_usage marks the PIC offset reg - call used. */ + call used or fixed. */ + if (crtl->calls_eh_return + && ((DEFAULT_ABI == ABI_V4 && flag_pic) + || (DEFAULT_ABI == ABI_DARWIN && flag_pic) + || (TARGET_TOC && TARGET_MINIMAL_TOC))) + return true; + if (TARGET_TOC && TARGET_MINIMAL_TOC - && (crtl->calls_eh_return - || df_regs_ever_live_p (reg) - || !constant_pool_empty_p ())) + && !constant_pool_empty_p ()) return true; - if ((DEFAULT_ABI == ABI_V4 || DEFAULT_ABI == ABI_DARWIN) + if (DEFAULT_ABI == ABI_DARWIN && flag_pic && crtl->uses_pic_offset_table) return true; + return false; } return !call_used_regs[reg] && df_regs_ever_live_p (reg);
Fixes lack of r30 save/restore on // -m32 -fpic -ftls-model=initial-exec __thread char* p; char** f1 (void) { return &p; } and // -m32 -fpic -msecure-plt extern int foo (int); int f1 (int x) { return foo (x); } These are both caused by save_reg_p returning false when the pic offset table reg (r30 for ABI_V4) was used, due to the logic not exactly matching that in rs6000_emit_prologue to set up r30. I've simplified the ABI_V4 logic down to df_regs_ever_live_p since that is obviously correct for all ABIs. I also noticed that save_reg_p isn't following the comment regarding calls_eh_return (since svn 267049, git 0edf78b1b2a0), and the comment needs tweaking too. For why the revised comment is correct, grep for saves_all_registers in lra.c, and yes, we do want to save the pic offset table reg for eh_return. Bootstrap and regression test on powerpc64-linux biarch in progress. OK assuming no regressions? Segher this patch supercedes https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00307.html as there's no need to set crtl->uses_pic_offset_table for those cases now with the df_regs_ever_live_p test in save_reg_p. PR target/88343 * config/rs6000/rs6000.c (save_reg_p): Correct calls_eh_return case. Match logic in rs6000_emit_prologue emitting pic_offset_table setup, simplifying ABI_V4 case to df_regs_ever_live_p.