Message ID | 20150224192919.GP1746@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
> Hi! > > As mentioned in the PR, the ix86_cmodel option is TargetSave, > but during option processing is adjusted from flag_pic, which is > for LTO a global option. > This causes a problem when some translation unit is compiled with LTO > without -fpic, and then the final link is done with -fpic - then > ix86_cmodel might be the non-_PIC CM_* even when flag_pic is on, which > greatly confuses the backend. > > Fixed by adding a target hook to do such adjustments upon TARGET_OPTION_NODE > streaming in. > > Unfortunately, for some reason I can't now really reproduce the ICE and the > testcase seems to be missing a function definition, but I've at least > verified it that without the patch there is inconsistent value of > ix86_cmodel while the patch fixes that. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2015-02-24 Jakub Jelinek <jakub@redhat.com> > > PR lto/64374 > * target.def (target_option_stream_in): New target hook. > * tree-streamer-in.c (streamer_read_tree_bitfields): Invoke > targetm.target_option.post_stream_in if non-NULL. > * doc/tm.texi.in: Add @hook TARGET_OPTION_POST_STREAM_IN. > * doc/tm.texi: Updated. > * config/i386/i386.c (ix86_function_specific_post_stream_in): New > function. > (TARGET_OPTION_POST_STREAM_IN): Redefine. Thanks, the i386 parts of the patch are OK, but I think you want to add the reverse transformation, too. I.e. if someone compiles with -fPIC but links without. My plan to fix the testcase was to put it into ix86_function_specific_restore which would save need for a new hook. But I am fine either way (just can't approve the newhook) Honza
On Tue, Feb 24, 2015 at 08:48:19PM +0100, Jan Hubicka wrote: > Thanks, the i386 parts of the patch are OK, but I think you want to add the reverse > transformation, too. I.e. if someone compiles with -fPIC but links without. I've only done it this way because that is what ix86_option_override_internal was doing, but supposedly only because the command line option is only about the non-PIC variants. So I agree that the other direction makes sense too and will adjust it. > My plan to fix the testcase was to put it into ix86_function_specific_restore > which would save need for a new hook. But I am fine either way (just can't > approve the newhook) The way the streaming in now works is that we don't have a gcc_options structure anywhere, so if it was done in the *_restore hook, you'd need to *_save it first and then restore. Richard, are you ok with the new hook? Jakub
> On Tue, Feb 24, 2015 at 08:48:19PM +0100, Jan Hubicka wrote: > > Thanks, the i386 parts of the patch are OK, but I think you want to add the reverse > > transformation, too. I.e. if someone compiles with -fPIC but links without. > > I've only done it this way because that is what > ix86_option_override_internal was doing, but supposedly only because the > command line option is only about the non-PIC variants. > So I agree that the other direction makes sense too and will adjust it. > > > My plan to fix the testcase was to put it into ix86_function_specific_restore > > which would save need for a new hook. But I am fine either way (just can't > > approve the newhook) > > The way the streaming in now works is that we don't have a gcc_options > structure anywhere, so if it was done in the *_restore hook, you'd need > to *_save it first and then restore. I see, we call cl_target_option_restore but not the hook. Oh well, then. Cleaning this up for GCC 6 would be amazing - I am sure we can do with fewer hooks and less of target code. Honza > > Richard, are you ok with the new hook? > > Jakub
On Tue, 24 Feb 2015, Jakub Jelinek wrote: > On Tue, Feb 24, 2015 at 08:48:19PM +0100, Jan Hubicka wrote: > > Thanks, the i386 parts of the patch are OK, but I think you want to add the reverse > > transformation, too. I.e. if someone compiles with -fPIC but links without. > > I've only done it this way because that is what > ix86_option_override_internal was doing, but supposedly only because the > command line option is only about the non-PIC variants. > So I agree that the other direction makes sense too and will adjust it. > > > My plan to fix the testcase was to put it into ix86_function_specific_restore > > which would save need for a new hook. But I am fine either way (just can't > > approve the newhook) > > The way the streaming in now works is that we don't have a gcc_options > structure anywhere, so if it was done in the *_restore hook, you'd need > to *_save it first and then restore. > > Richard, are you ok with the new hook? Yeah. Can't think of a better way (other than not doing what the x86 backend does). Richard.
--- gcc/target.def.jj 2015-02-23 10:43:03.000000000 +0100 +++ gcc/target.def 2015-02-24 14:54:25.061494421 +0100 @@ -5501,6 +5501,15 @@ information in the @code{struct cl_targe function-specific options to the @code{struct gcc_options} structure.", void, (struct gcc_options *opts, struct cl_target_option *ptr), NULL) +/* Function to update target-specific option information after being + streamed in. */ +DEFHOOK +(post_stream_in, + "This hook is called to update target-specific information in the\n\ +@code{struct cl_target_option} structure after it is streamed in from\n\ +LTO bytecode.", + void, (struct cl_target_option *ptr), NULL) + /* Function to print any extra target state from the target options structure. */ DEFHOOK --- gcc/tree-streamer-in.c.jj 2015-02-23 10:43:04.000000000 +0100 +++ gcc/tree-streamer-in.c 2015-02-24 15:40:14.470602155 +0100 @@ -559,7 +559,11 @@ streamer_read_tree_bitfields (struct lto #ifndef ACCEL_COMPILER if (CODE_CONTAINS_STRUCT (code, TS_TARGET_OPTION)) - cl_target_option_stream_in (data_in, &bp, TREE_TARGET_OPTION (expr)); + { + cl_target_option_stream_in (data_in, &bp, TREE_TARGET_OPTION (expr)); + if (targetm.target_option.post_stream_in) + targetm.target_option.post_stream_in (TREE_TARGET_OPTION (expr)); + } #endif if (code == OMP_CLAUSE) --- gcc/doc/tm.texi.in.jj 2015-02-23 10:43:03.000000000 +0100 +++ gcc/doc/tm.texi.in 2015-02-24 15:19:14.919179569 +0100 @@ -7251,6 +7251,8 @@ on this implementation detail. @hook TARGET_OPTION_RESTORE +@hook TARGET_OPTION_POST_STREAM_IN + @hook TARGET_OPTION_PRINT @hook TARGET_OPTION_PRAGMA_PARSE --- gcc/doc/tm.texi.jj 2015-02-23 10:43:03.000000000 +0100 +++ gcc/doc/tm.texi 2015-02-24 15:19:42.038736514 +0100 @@ -9905,6 +9905,12 @@ information in the @code{struct cl_targe function-specific options to the @code{struct gcc_options} structure. @end deftypefn +@deftypefn {Target Hook} void TARGET_OPTION_POST_STREAM_IN (struct cl_target_option *@var{ptr}) +This hook is called to update target-specific information in the +@code{struct cl_target_option} structure after it is streamed in from +LTO bytecode. +@end deftypefn + @deftypefn {Target Hook} void TARGET_OPTION_PRINT (FILE *@var{file}, int @var{indent}, struct cl_target_option *@var{ptr}) This hook is called to print any additional target-specific information in the @code{struct cl_target_option} structure for --- gcc/config/i386/i386.c.jj 2015-02-23 10:43:04.000000000 +0100 +++ gcc/config/i386/i386.c 2015-02-24 15:18:18.495101374 +0100 @@ -2463,6 +2463,7 @@ static void ix86_function_specific_save struct gcc_options *opts); static void ix86_function_specific_restore (struct gcc_options *opts, struct cl_target_option *); +static void ix86_function_specific_post_stream_in (struct cl_target_option *); static void ix86_function_specific_print (FILE *, int, struct cl_target_option *); static bool ix86_valid_target_attribute_p (tree, tree, tree, int); @@ -4571,6 +4572,39 @@ ix86_function_specific_restore (struct g set_ix86_tune_features (ix86_tune, false); } +/* Adjust target options after streaming them in. This is mainly about + reconciling them with global options. */ + +static void +ix86_function_specific_post_stream_in (struct cl_target_option *ptr) +{ + /* flag_pic is a global option, but ix86_cmodel is target saved option + partly computed from flag_pic. If flag_pic is on, adjust x_ix86_cmodel + for PIC, or error out. */ + if (flag_pic) + switch (ptr->x_ix86_cmodel) + { + case CM_SMALL: + ptr->x_ix86_cmodel = CM_SMALL_PIC; + break; + + case CM_MEDIUM: + ptr->x_ix86_cmodel = CM_MEDIUM_PIC; + break; + + case CM_LARGE: + ptr->x_ix86_cmodel = CM_LARGE_PIC; + break; + + case CM_KERNEL: + error ("code model %s does not support PIC mode", "kernel"); + break; + + default: + break; + } +} + /* Print the current options */ static void @@ -52007,6 +52041,9 @@ ix86_initialize_bounds (tree var, tree l #undef TARGET_OPTION_RESTORE #define TARGET_OPTION_RESTORE ix86_function_specific_restore +#undef TARGET_OPTION_POST_STREAM_IN +#define TARGET_OPTION_POST_STREAM_IN ix86_function_specific_post_stream_in + #undef TARGET_OPTION_PRINT #define TARGET_OPTION_PRINT ix86_function_specific_print