[PATCH] rs6000: Fix huge compile time on thunks (PR64580)

[PATCH] rs6000: Fix huge compile time on thunks (PR64580)

[Segher Boessenkool]

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.

---
 gcc/config/rs6000/rs6000.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

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.  */
-- 
1.8.1.4

Re: [PATCH] rs6000: Fix huge compile time on thunks (PR64580)

[David Edelsohn]

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

Re: [PATCH] rs6000: Fix huge compile time on thunks (PR64580)

[Segher Boessenkool]

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

Re: [PATCH] rs6000: Fix huge compile time on thunks (PR64580)

[David Edelsohn]

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