Patchwork Add a prepare_pch_save target hook

login
register
mail settings
Submitter Richard Sandiford
Date Dec. 4, 2011, 12:02 p.m.
Message ID <87obvoee8x.fsf@firetop.home>
Download mbox | patch
Permalink /patch/129145/
State New
Headers show

Comments

Richard Sandiford - Dec. 4, 2011, 12:02 p.m.
A while back I added the target_globals structure, to allow a backend
to switch between two very different ISA modes without paying the full
target_reinit penalty each time.  This made a huge difference to compile
time, but had a drawback: the target_globals structure contained both
GGC and non-GGC data.  This meant that secondary target_globals structures
like mips16_globals couldn't be saved correctly in PCH files.

I'm afraid I was aware of this at the time, but decided to go ahead
anyway on the basis that compiling MIPS16 code in reasonable time was
more important than creating MIPS16 PCH files.  But after um-ing and
ah-ing about the best fix, I've finally settled on the patch below.

Tested on mips64-linux-gnu, where it fixes many PCH failures for MIPS16.
OK to install?

Richard


gcc/
	* doc/tm.texi.in (TARGET_PREPARE_PCH_SAVE): New hook.
	* doc/tm.texi: Regenerate.
	* target.def (prepare_pch_save): New hook.
	* c-family/c-pch.c (c_common_write_pch): Call it.
	* config/mips/mips.c (was_mips16_pch_p): Delete.
	(mips_set_mips16_mode): Don't refer to was_mips16_pch_p.
	(mips_prepare_pch_save): New function.
	(TARGET_PREPARE_PCH_SAVE): Define.
Mike Stump - Dec. 4, 2011, 4:52 p.m.
On Dec 4, 2011, at 4:02 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote:
> A while back I added the target_globals structure, to allow a backend
> to switch between two very different ISA modes without paying the full
> target_reinit penalty each time.  This made a huge difference to compile
> time, but had a drawback: the target_globals structure contained both
> GGC and non-GGC data.  This meant that secondary target_globals structures
> like mips16_globals couldn't be saved correctly in PCH files.

> +  /* We are called in a context where the current MIPS16 vs. non-MIPS16
> +     setting should be irrelevant.  The question then is: which setting
> +     makes most sense at load time?
> +
> +     The PCH is loaded before the first token is read.  We should never
> +     have switched into MIPS16 mode by that point,

If there is any way to say:

  #pragma mips16

to globally switch state into mips16 mode, then I believe the patch is wrong.  The pch file can have that directive in it to put the compiler into that mode, with all the resulting state that mode entails.  Now, if you store that one bit into the pch file, say, via a GTY int flag, then on reload, you can examine that flag, and enter that state.  Bear in mind, this 'slows' pch load time.
Richard Sandiford - Dec. 5, 2011, 9:13 a.m.
Mike Stump <mikestump@comcast.net> writes:
> On Dec 4, 2011, at 4:02 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote:
>> A while back I added the target_globals structure, to allow a backend
>> to switch between two very different ISA modes without paying the full
>> target_reinit penalty each time.  This made a huge difference to compile
>> time, but had a drawback: the target_globals structure contained both
>> GGC and non-GGC data.  This meant that secondary target_globals structures
>> like mips16_globals couldn't be saved correctly in PCH files.
>
>> +  /* We are called in a context where the current MIPS16 vs. non-MIPS16
>> +     setting should be irrelevant.  The question then is: which setting
>> +     makes most sense at load time?
>> +
>> +     The PCH is loaded before the first token is read.  We should never
>> +     have switched into MIPS16 mode by that point,
>
> If there is any way to say:
>
>   #pragma mips16
>
> to globally switch state into mips16 mode, then I believe the patch is
> wrong.

The global switching of state happens when we start to compile a function,
which means we must have seen a C token.  The target_globals state is
non-MIPS16 up until then, even if -mips16 is passed on the command line,
or if the command line is changed by pragmas.

Besides, I think:

         #pragma GCC optimize ...X...
         #include "foo.h"

breaks the PCH contract if X is incompatible with the flags used at
the beginning of the PCH compilation.

Richard

Patch

Index: gcc/doc/tm.texi.in
===================================================================
--- gcc/doc/tm.texi.in	2011-12-04 08:52:33.000000000 +0000
+++ gcc/doc/tm.texi.in	2011-12-04 11:30:12.000000000 +0000
@@ -9974,6 +9974,8 @@  of @code{target_flags}.  @var{pch_flags}
 value is the same as for @code{TARGET_PCH_VALID_P}.
 @end deftypefn
 
+@hook TARGET_PREPARE_PCH_SAVE
+
 @node C++ ABI
 @section C++ ABI parameters
 @cindex parameters, c++ abi
Index: gcc/doc/tm.texi
===================================================================
--- gcc/doc/tm.texi	2011-12-04 08:52:33.000000000 +0000
+++ gcc/doc/tm.texi	2011-12-04 11:30:12.000000000 +0000
@@ -10079,6 +10079,13 @@  of @code{target_flags}.  @var{pch_flags}
 value is the same as for @code{TARGET_PCH_VALID_P}.
 @end deftypefn
 
+@deftypefn {Target Hook} void TARGET_PREPARE_PCH_SAVE (void)
+Called before writing out a PCH file.  If the target has some
+garbage-collected data that needs to be in a particular state on PCH loads,
+it can use this hook to enforce that state.  Very few targets need
+to do anything here.
+@end deftypefn
+
 @node C++ ABI
 @section C++ ABI parameters
 @cindex parameters, c++ abi
Index: gcc/target.def
===================================================================
--- gcc/target.def	2011-12-04 08:52:33.000000000 +0000
+++ gcc/target.def	2011-12-04 11:30:12.000000000 +0000
@@ -1819,6 +1819,15 @@  DEFHOOK
  const char *, (const void *data, size_t sz),
  default_pch_valid_p)
 
+DEFHOOK
+(prepare_pch_save,
+ "Called before writing out a PCH file.  If the target has some\n\
+garbage-collected data that needs to be in a particular state on PCH loads,\n\
+it can use this hook to enforce that state.  Very few targets need\n\
+to do anything here.",
+ void, (void),
+ hook_void_void)
+
 /* If nonnull, this function checks whether a PCH file with the
    given set of target flags can be used.  It returns NULL if so,
    otherwise it returns an error message.  */
Index: gcc/c-family/c-pch.c
===================================================================
--- gcc/c-family/c-pch.c	2011-12-04 08:52:33.000000000 +0000
+++ gcc/c-family/c-pch.c	2011-12-04 11:30:12.000000000 +0000
@@ -180,6 +180,8 @@  c_common_write_pch (void)
 
   timevar_push (TV_PCH_SAVE);
 
+  targetm.prepare_pch_save ();
+
   (*debug_hooks->handle_pch) (1);
 
   cpp_write_pch_deps (parse_in, pch_outfile);
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	2011-12-04 08:52:32.000000000 +0000
+++ gcc/config/mips/mips.c	2011-12-04 12:01:44.000000000 +0000
@@ -15197,14 +15197,8 @@  mips_output_mi_thunk (FILE *file, tree t
 }
 
 /* The last argument passed to mips_set_mips16_mode, or negative if the
-   function hasn't been called yet.
-
-   There are two copies of this information.  One is saved and restored
-   by the PCH process while the other is specific to this compiler
-   invocation.  The information calculated by mips_set_mips16_mode
-   is invalid unless the two variables are the same.  */
+   function hasn't been called yet.  */
 static int was_mips16_p = -1;
-static GTY(()) int was_mips16_pch_p = -1;
 
 /* Set up the target-dependent global state so that it matches the
    current function's ISA mode.  */
@@ -15212,8 +15206,7 @@  static GTY(()) int was_mips16_pch_p = -1
 static void
 mips_set_mips16_mode (int mips16_p)
 {
-  if (mips16_p == was_mips16_p
-      && mips16_p == was_mips16_pch_p)
+  if (mips16_p == was_mips16_p)
     return;
 
   /* Restore base settings of various flags.  */
@@ -15310,7 +15303,6 @@  mips_set_mips16_mode (int mips16_p)
     restore_target_globals (&default_target_globals);
 
   was_mips16_p = mips16_p;
-  was_mips16_pch_p = mips16_p;
 }
 
 /* Implement TARGET_SET_CURRENT_FUNCTION.  Decide whether the current
@@ -16325,6 +16317,34 @@  mips_shift_truncation_mask (enum machine
   return GET_MODE_BITSIZE (mode) - 1;
 }
 
+/* Implement TARGET_PREPARE_PCH_SAVE.  */
+
+static void
+mips_prepare_pch_save (void)
+{
+  /* We are called in a context where the current MIPS16 vs. non-MIPS16
+     setting should be irrelevant.  The question then is: which setting
+     makes most sense at load time?
+
+     The PCH is loaded before the first token is read.  We should never
+     have switched into MIPS16 mode by that point, and thus should not
+     have populated mips16_globals.  Nor can we load the entire contents
+     of mips16_globals from the PCH file, because mips16_globals contains
+     a combination of GGC and non-GGC data.
+
+     There is therefore no point in trying save the GGC part of
+     mips16_globals to the PCH file, or to preserve MIPS16ness across
+     the PCH save and load.  The loading compiler would not have access
+     to the non-GGC parts of mips16_globals (either from the PCH file,
+     or from a copy that the loading compiler generated itself) and would
+     have to call target_reinit anyway.
+
+     It therefore seems best to switch back to non-MIPS16 mode at
+     save time, and to ensure that mips16_globals remains null after
+     a PCH load.  */
+  mips_set_mips16_mode (false);
+  mips16_globals = 0;
+}
 
 /* Initialize the GCC target structure.  */
 #undef TARGET_ASM_ALIGNED_HI_OP
@@ -16544,6 +16564,9 @@  #define TARGET_ASM_OUTPUT_SOURCE_FILENAM
 #undef TARGET_SHIFT_TRUNCATION_MASK
 #define TARGET_SHIFT_TRUNCATION_MASK mips_shift_truncation_mask
 
+#undef TARGET_PREPARE_PCH_SAVE
+#define TARGET_PREPARE_PCH_SAVE mips_prepare_pch_save
+
 struct gcc_target targetm = TARGET_INITIALIZER;
 
 #include "gt-mips.h"