Change i?86/x86_64 into SWITCHABLE_TARGET (PR58115)

On Mon, Jan 06, 2014 at 10:27:06AM +0000, Richard Sandiford wrote:
> Of course, IMO, the cleanest fix would be to use switchable targets
> for i386...

The following patch does that, bootstrapped/regtested on x86_64-linux and
i686-linux.  The only problem with the patch is PCH,
+FAIL: 17_intro/headers/c++200x/stdc++.cc (test for excess errors)
+FAIL: 17_intro/headers/c++200x/stdc++_multiple_inclusion.cc (test for excess errors)
(both 32-bit and 64-bit regtests), where it ICEs.  I guess the problem is
that the target globals are allocated partly in GC, partly in heap and
even if they were allocated completely in GC and GTY(()) marked fully all
the individual pointed structures, we IMNSHO still don't want it to be
saved during PCH and restored later, what we have is basically just a cache
of the target globals.

Dunno what is the best way to handle that though.
Either before writing PCH c-common.c could call some tree.c routine that
would traverse the cl_option_hash_table hash table and for every
TARGET_OPTION_NODE in the hash table clear TREE_TARGET_GLOBALS.
Or perhaps some gengtype extension to run some routine before PCH saving
on the tree_target_option structs and clear the globals field in there.
Or use GTY((user)) on tree_target_option, but then dunno how we'd handle the
marking of the embedded opts field (and common).
Any ideas?

2014-01-07  Jakub Jelinek  <jakub@redhat.com>

	PR target/58115
	* tree-core.h (struct target_globals): New forward declaration.
	(struct tree_target_option): Add globals field.
	* tree.h (TREE_TARGET_GLOBALS): Define.
	* target-globals.h (struct target_globals): Define even if
	!SWITCHABLE_TARGET.
	* config/i386/i386.h (SWITCHABLE_TARGET): Define.
	* config/i386/i386.c: Include target-globals.h.
	(ix86_set_current_function): Instead of doing target_reinit
	unconditionally, use save_target_globals_default_opts and
	restore_target_globals.



	Jakub

Jakub Jelinek <jakub@redhat.com> writes:
> On Mon, Jan 06, 2014 at 10:27:06AM +0000, Richard Sandiford wrote:
>> Of course, IMO, the cleanest fix would be to use switchable targets
>> for i386...
>
> The following patch does that, bootstrapped/regtested on x86_64-linux and
> i686-linux.

Neat, thanks.  Looks like it's a lot less intrusive than I thought it
might be.

> The only problem with the patch is PCH,
> +FAIL: 17_intro/headers/c++200x/stdc++.cc (test for excess errors)
> +FAIL: 17_intro/headers/c++200x/stdc++_multiple_inclusion.cc (test for excess errors)
> (both 32-bit and 64-bit regtests), where it ICEs.  I guess the problem is
> that the target globals are allocated partly in GC, partly in heap and
> even if they were allocated completely in GC and GTY(()) marked fully all
> the individual pointed structures, we IMNSHO still don't want it to be
> saved during PCH and restored later, what we have is basically just a cache
> of the target globals.
>
> Dunno what is the best way to handle that though.
> Either before writing PCH c-common.c could call some tree.c routine that
> would traverse the cl_option_hash_table hash table and for every
> TARGET_OPTION_NODE in the hash table clear TREE_TARGET_GLOBALS.
> Or perhaps some gengtype extension to run some routine before PCH saving
> on the tree_target_option structs and clear the globals field in there.
> Or use GTY((user)) on tree_target_option, but then dunno how we'd handle the
> marking of the embedded opts field (and common).
> Any ideas?

Not really.  For MIPS this was relatively easy since there was only one
non-default instance:

/* 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_compression_mode (0);
  mips16_globals = 0;
}

The idea of iterating over cl_option_hash_table sounded good though.
It might also be the easiest option.

Hopefully at some point we can move the main body of
ix86_set_current_function into generic code, but that's probably
4.10 material.

Thanks,
Richard

On Tue, Jan 7, 2014 at 8:39 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Jan 06, 2014 at 10:27:06AM +0000, Richard Sandiford wrote:
>> Of course, IMO, the cleanest fix would be to use switchable targets
>> for i386...
>
> The following patch does that, bootstrapped/regtested on x86_64-linux and
> i686-linux.  The only problem with the patch is PCH,
> +FAIL: 17_intro/headers/c++200x/stdc++.cc (test for excess errors)
> +FAIL: 17_intro/headers/c++200x/stdc++_multiple_inclusion.cc (test for excess errors)
> (both 32-bit and 64-bit regtests), where it ICEs.  I guess the problem is
> that the target globals are allocated partly in GC, partly in heap and
> even if they were allocated completely in GC and GTY(()) marked fully all
> the individual pointed structures, we IMNSHO still don't want it to be
> saved during PCH and restored later, what we have is basically just a cache
> of the target globals.
>
> Dunno what is the best way to handle that though.
> Either before writing PCH c-common.c could call some tree.c routine that
> would traverse the cl_option_hash_table hash table and for every
> TARGET_OPTION_NODE in the hash table clear TREE_TARGET_GLOBALS.
> Or perhaps some gengtype extension to run some routine before PCH saving
> on the tree_target_option structs and clear the globals field in there.
> Or use GTY((user)) on tree_target_option, but then dunno how we'd handle the
> marking of the embedded opts field (and common).
> Any ideas?

Well, a GTY((skip_pch)) would probably work.  Or move the thing
out-of GC land (thus make cl_option_hash_table persistant) and
simply GTY((skip)) the pointer completely.  Not sure if we ever
collect from it.

Richard.

> 2014-01-07  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/58115
>         * tree-core.h (struct target_globals): New forward declaration.
>         (struct tree_target_option): Add globals field.
>         * tree.h (TREE_TARGET_GLOBALS): Define.
>         * target-globals.h (struct target_globals): Define even if
>         !SWITCHABLE_TARGET.
>         * config/i386/i386.h (SWITCHABLE_TARGET): Define.
>         * config/i386/i386.c: Include target-globals.h.
>         (ix86_set_current_function): Instead of doing target_reinit
>         unconditionally, use save_target_globals_default_opts and
>         restore_target_globals.
>
> --- gcc/tree-core.h.jj  2014-01-07 08:47:24.000000000 +0100
> +++ gcc/tree-core.h     2014-01-07 16:44:35.591358235 +0100
> @@ -1557,11 +1557,18 @@ struct GTY(()) tree_optimization_option
>    struct target_optabs *GTY ((skip)) base_optabs;
>  };
>
> +/* Forward declaration, defined in target-globals.h.  */
> +
> +struct GTY(()) target_globals;
> +
>  /* Target options used by a function.  */
>
>  struct GTY(()) tree_target_option {
>    struct tree_common common;
>
> +  /* Target globals for the corresponding target option.  */
> +  struct target_globals *globals;
> +
>    /* The optimization options used by the user.  */
>    struct cl_target_option opts;
>  };
> --- gcc/tree.h.jj       2014-01-03 11:40:33.000000000 +0100
> +++ gcc/tree.h  2014-01-07 12:55:39.137295100 +0100
> @@ -2695,6 +2695,9 @@ extern tree build_optimization_node (str
>  #define TREE_TARGET_OPTION(NODE) \
>    (&TARGET_OPTION_NODE_CHECK (NODE)->target_option.opts)
>
> +#define TREE_TARGET_GLOBALS(NODE) \
> +  (TARGET_OPTION_NODE_CHECK (NODE)->target_option.globals)
> +
>  /* Return a tree node that encapsulates the target options in OPTS.  */
>  extern tree build_target_option_node (struct gcc_options *opts);
>
> --- gcc/target-globals.h.jj     2014-01-03 11:40:46.000000000 +0100
> +++ gcc/target-globals.h        2014-01-07 17:08:51.113880947 +0100
> @@ -37,6 +37,7 @@ extern struct target_builtins *this_targ
>  extern struct target_gcse *this_target_gcse;
>  extern struct target_bb_reorder *this_target_bb_reorder;
>  extern struct target_lower_subreg *this_target_lower_subreg;
> +#endif
>
>  struct GTY(()) target_globals {
>    struct target_flag_state *GTY((skip)) flag_state;
> @@ -57,6 +58,7 @@ struct GTY(()) target_globals {
>    struct target_lower_subreg *GTY((skip)) lower_subreg;
>  };
>
> +#if SWITCHABLE_TARGET
>  extern struct target_globals default_target_globals;
>
>  extern struct target_globals *save_target_globals (void);
> --- gcc/config/i386/i386.h.jj   2014-01-06 22:37:19.000000000 +0100
> +++ gcc/config/i386/i386.h      2014-01-07 12:13:06.480486755 +0100
> @@ -2510,6 +2510,9 @@ extern void debug_dispatch_window (int);
>  #define IX86_HLE_ACQUIRE (1 << 16)
>  #define IX86_HLE_RELEASE (1 << 17)
>
> +/* For switching between functions with different target attributes.  */
> +#define SWITCHABLE_TARGET 1
> +
>  /*
>  Local variables:
>  version-control: t
> --- gcc/config/i386/i386.c.jj   2014-01-06 22:37:19.000000000 +0100
> +++ gcc/config/i386/i386.c      2014-01-07 16:52:32.597904760 +0100
> @@ -80,6 +80,7 @@ along with GCC; see the file COPYING3.
>  #include "tree-pass.h"
>  #include "context.h"
>  #include "pass_manager.h"
> +#include "target-globals.h"
>
>  static rtx legitimize_dllimport_symbol (rtx, bool);
>  static rtx legitimize_pe_coff_extern_decl (rtx, bool);
> @@ -4868,16 +4869,25 @@ ix86_set_current_function (tree fndecl)
>         {
>           cl_target_option_restore (&global_options,
>                                     TREE_TARGET_OPTION (new_tree));
> -         target_reinit ();
> +         if (TREE_TARGET_GLOBALS (new_tree))
> +           restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
> +         else
> +           TREE_TARGET_GLOBALS (new_tree)
> +             = save_target_globals_default_opts ();
>         }
>
>        else if (old_tree)
>         {
> -         struct cl_target_option *def
> -           = TREE_TARGET_OPTION (target_option_current_node);
> -
> -         cl_target_option_restore (&global_options, def);
> -         target_reinit ();
> +         new_tree = target_option_current_node;
> +         cl_target_option_restore (&global_options,
> +                                   TREE_TARGET_OPTION (new_tree));
> +         if (TREE_TARGET_GLOBALS (new_tree))
> +           restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
> +         else if (new_tree == target_option_default_node)
> +           restore_target_globals (&default_target_globals);
> +         else
> +           TREE_TARGET_GLOBALS (new_tree)
> +             = save_target_globals_default_opts ();
>         }
>      }
>  }
>
>
>         Jakub