| Message ID | b7bb380a7300a80eef89045310280b3287f7724f.1422562775.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On Thu, Jan 29, 2015 at 7:38 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote: > The problem in this PR is that rs6000_stack_info spends an enormous amount > of time when compiling thunks. This turns out to be a loop in > compute_vrsave_mask that loops (almost) 4G times because the loop counter > is unsigned and the reversed loop is not written in a way that can handle > crtl->args.info.vregno == 0. Make it a forward loop free of such traps, > which is easier to read anyway. > > But, vregno should never be 0 if rs6000_stack_info is called; it means > that various data is not initialised for this function. Which is correct, > this is a thunk: rs6000_stack_info should not be called for thunks. Add > an assert for that. > > rs6000_output_function_prologue (the thing outputting asm stuff) calls > it though. Don't do the savres thing for thunks (they never need it). > Factor the relevant code to a new function while at it. > > rs6000_output_function_epilogue already guards properly with is_thunk, > and the assert didn't fire on a bootstrap+testrun, so it seems this was > the only place the mistake happened. > > > Bootstrapped and tested as usual; okay for mainline? And release branches > if it happens there (the PR doesn't say yet, it's not marked as regression > either, looking at history it seems to go way way back)? > > > Segher > > > 2015-01-29 Segher Boessenkool <segher@kernel.crashing.org> > > gcc/ > PR target/64580 > * config.rs6000/rs6000.c (compute_vrsave_mask): Reverse loop order. > (rs6000_stack_info): Add assert. > (rs6000_output_savres_externs): New function, split off from... > (rs6000_output_function_prologue): ... here. Do not call it for > thunks. Okay. Thanks, David
On Thu, Jan 29, 2015 at 04:38:21PM -0800, Segher Boessenkool wrote: > 2015-01-29 Segher Boessenkool <segher@kernel.crashing.org> > > gcc/ > PR target/64580 > * config.rs6000/rs6000.c (compute_vrsave_mask): Reverse loop order. > (rs6000_stack_info): Add assert. > (rs6000_output_savres_externs): New function, split off from... > (rs6000_output_function_prologue): ... here. Do not call it for > thunks. Hi David, Is this okay to backport to 4.9 and 4.8? Bootstrapped and tested on both. Segher
On Thu, Feb 5, 2015 at 9:50 AM, Segher Boessenkool <segher@kernel.crashing.org> wrote: > On Thu, Jan 29, 2015 at 04:38:21PM -0800, Segher Boessenkool wrote: >> 2015-01-29 Segher Boessenkool <segher@kernel.crashing.org> >> >> gcc/ >> PR target/64580 >> * config.rs6000/rs6000.c (compute_vrsave_mask): Reverse loop order. >> (rs6000_stack_info): Add assert. >> (rs6000_output_savres_externs): New function, split off from... >> (rs6000_output_function_prologue): ... here. Do not call it for >> thunks. > > Hi David, > > Is this okay to backport to 4.9 and 4.8? Bootstrapped and tested on both. Yes, please backport the patch to those open branches. And, again, thanks for tracking down and fixing this! Thanks, David
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 2679570..be9b236 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -21182,7 +21182,7 @@ compute_vrsave_mask (void) them in again. More importantly, the mask we compute here is used to generate CLOBBERs in the set_vrsave insn, and we do not wish the argument registers to die. */ - for (i = crtl->args.info.vregno - 1; i >= ALTIVEC_ARG_MIN_REG; --i) + for (i = ALTIVEC_ARG_MIN_REG; i < (unsigned) crtl->args.info.vregno; i++) mask &= ~ALTIVEC_REG_BIT (i); /* Similarly, remove the return value from the set. */ @@ -21591,6 +21591,9 @@ rs6000_savres_strategy (rs6000_stack_t *info, static rs6000_stack_t * rs6000_stack_info (void) { + /* We should never be called for thunks, we are not set up for that. */ + gcc_assert (!cfun->is_thunk); + rs6000_stack_t *info_ptr = &stack_info; int reg_size = TARGET_32BIT ? 4 : 8; int ehrd_size; @@ -24312,11 +24315,10 @@ rs6000_emit_prologue (void) } } -/* Write function prologue. */ +/* Output .extern statements for the save/restore routines we use. */ static void -rs6000_output_function_prologue (FILE *file, - HOST_WIDE_INT size ATTRIBUTE_UNUSED) +rs6000_output_savres_externs (FILE *file) { rs6000_stack_t *info = rs6000_stack_info (); @@ -24348,6 +24350,16 @@ rs6000_output_function_prologue (FILE *file, fprintf (file, "\t.extern %s\n", name); } } +} + +/* Write function prologue. */ + +static void +rs6000_output_function_prologue (FILE *file, + HOST_WIDE_INT size ATTRIBUTE_UNUSED) +{ + if (!cfun->is_thunk) + rs6000_output_savres_externs (file); /* ELFv2 ABI r2 setup code and local entry point. This must follow immediately after the global entry point label. */