[PATCH] Fix LTO option streaming

[PATCH] Fix LTO option streaming

[Richard Biener]

It looks like when transitioning to using target and optimization
option nodes for compile-time to link-time option streaming you
didn't adjust lto-opts.c nor lto-wrapper.c.  The following fixes
the target option side (for SWITCHABLE_TARGET).  Do not record
any target options in the lto_opts section.

Honza - I suppose we don't have any testcase that this works, I'll
try to come up with sth.  This also looks like a correctness issue
to me.

We can do similar changes for optimize options, now, for all targets, no?

Thanks,
Richard.

2015-04-23  Richard Biener  <rguenther@suse.de>

	* lto-opts.c (lto_write_options): Do not record target options
	if we are targeting a SWITCHABLE_TARGET target.  We use target
	options on functions to transfer target flags from compile to
	link time.

Index: gcc/lto-opts.c
===================================================================
*** gcc/lto-opts.c	(revision 222360)
--- gcc/lto-opts.c	(working copy)
*************** lto_write_options (void)
*** 219,224 ****
--- 219,230 ----
  	  && lto_stream_offload_p)
         continue;
  
+       /* Do not store target-specific options if we target a
+          SWITCHABLE_TARGET target.  */
+       if ((cl_options[option->opt_index].flags & CL_TARGET)
+ 	  && SWITCHABLE_TARGET)
+ 	continue;
+ 
        /* Drop options created from the gcc driver that will be rejected
  	 when passed on to the driver again.  */
        if (cl_options[option->opt_index].cl_reject_driver)

Re: [PATCH] Fix LTO option streaming

[Jan Hubicka]

> 
> It looks like when transitioning to using target and optimization
> option nodes for compile-time to link-time option streaming you
> didn't adjust lto-opts.c nor lto-wrapper.c.  The following fixes

Yep, I assumed that lto-wrapper's merging is now redundant for optimization
options, while it is still needed for global functions (-fPIC) that was
not covered by my changes and since this was all bit late, it seemed I can
just keep it for next stage1.

> the target option side (for SWITCHABLE_TARGET).  Do not record
> any target options in the lto_opts section.
> 
> Honza - I suppose we don't have any testcase that this works, I'll
> try to come up with sth.  This also looks like a correctness issue
> to me.

Thanks, that would be a good idea. So with SWITCHABLE_TARGET
we have problem that after switching the options are misinterpretted?
I think Jakub made patch to simply skip target-option-node streaming here,
how this is supposed to work.
> 
> We can do similar changes for optimize options, now, for all targets, no?

I am not quite sure about this; not all options are per-function nature.
I need to audit all the target options for LTO safety, I did some work
on this for optimization options, but the target options are still very wild.
For example I am not quite convinced mixing -malign-double -mno-align-double units
will work as expected.

Honza
> 
> Thanks,
> Richard.
> 
> 2015-04-23  Richard Biener  <rguenther@suse.de>
> 
> 	* lto-opts.c (lto_write_options): Do not record target options
> 	if we are targeting a SWITCHABLE_TARGET target.  We use target
> 	options on functions to transfer target flags from compile to
> 	link time.
> 
> Index: gcc/lto-opts.c
> ===================================================================
> *** gcc/lto-opts.c	(revision 222360)
> --- gcc/lto-opts.c	(working copy)
> *************** lto_write_options (void)
> *** 219,224 ****
> --- 219,230 ----
>   	  && lto_stream_offload_p)
>          continue;
>   
> +       /* Do not store target-specific options if we target a
> +          SWITCHABLE_TARGET target.  */
> +       if ((cl_options[option->opt_index].flags & CL_TARGET)
> + 	  && SWITCHABLE_TARGET)
> + 	continue;
> + 
>         /* Drop options created from the gcc driver that will be rejected
>   	 when passed on to the driver again.  */
>         if (cl_options[option->opt_index].cl_reject_driver)

Re: [PATCH] Fix LTO option streaming

[Richard Biener]

On Thu, 23 Apr 2015, Jan Hubicka wrote:

> > 
> > It looks like when transitioning to using target and optimization
> > option nodes for compile-time to link-time option streaming you
> > didn't adjust lto-opts.c nor lto-wrapper.c.  The following fixes
> 
> Yep, I assumed that lto-wrapper's merging is now redundant for optimization
> options, while it is still needed for global functions (-fPIC) that was
> not covered by my changes and since this was all bit late, it seemed I can
> just keep it for next stage1.
> 
> > the target option side (for SWITCHABLE_TARGET).  Do not record
> > any target options in the lto_opts section.
> > 
> > Honza - I suppose we don't have any testcase that this works, I'll
> > try to come up with sth.  This also looks like a correctness issue
> > to me.
> 
> Thanks, that would be a good idea. So with SWITCHABLE_TARGET
> we have problem that after switching the options are misinterpretted?
> I think Jakub made patch to simply skip target-option-node streaming here,
> how this is supposed to work.

Not all targets support target attributes and I think those that
support them are those that are SWITCHABLE_TARGET - but maybe I am
mistaken?

> > 
> > We can do similar changes for optimize options, now, for all targets, no?
> 
> I am not quite sure about this; not all options are per-function nature.
> I need to audit all the target options for LTO safety, I did some work
> on this for optimization options, but the target options are still very wild.
> For example I am not quite convinced mixing -malign-double -mno-align-double units
> will work as expected.

Well, but once types are laid out specifying sth else at link time
won't help.  But yes, if we don't stream the option we have no chance
to complain in lto-wrapper.  It's just that we seem to not enter
the merging machinery for single-file testcases so I chose to "fix"
lto option streaming rather than lto-wrapper ... I'll have a closer
look to lto-wrapper.  But to complain about target option mismatches
we'd have to call a target hook from lto-wrapper (or finally re-structure
all this and merge options in lto1 itself).

Richard.

> Honza
> > 
> > Thanks,
> > Richard.
> > 
> > 2015-04-23  Richard Biener  <rguenther@suse.de>
> > 
> > 	* lto-opts.c (lto_write_options): Do not record target options
> > 	if we are targeting a SWITCHABLE_TARGET target.  We use target
> > 	options on functions to transfer target flags from compile to
> > 	link time.
> > 
> > Index: gcc/lto-opts.c
> > ===================================================================
> > *** gcc/lto-opts.c	(revision 222360)
> > --- gcc/lto-opts.c	(working copy)
> > *************** lto_write_options (void)
> > *** 219,224 ****
> > --- 219,230 ----
> >   	  && lto_stream_offload_p)
> >          continue;
> >   
> > +       /* Do not store target-specific options if we target a
> > +          SWITCHABLE_TARGET target.  */
> > +       if ((cl_options[option->opt_index].flags & CL_TARGET)
> > + 	  && SWITCHABLE_TARGET)
> > + 	continue;
> > + 
> >         /* Drop options created from the gcc driver that will be rejected
> >   	 when passed on to the driver again.  */
> >         if (cl_options[option->opt_index].cl_reject_driver)
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild,
Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix LTO option streaming

[Richard Biener]

On Thu, 23 Apr 2015, Richard Biener wrote:

> On Thu, 23 Apr 2015, Jan Hubicka wrote:
> 
> > > 
> > > It looks like when transitioning to using target and optimization
> > > option nodes for compile-time to link-time option streaming you
> > > didn't adjust lto-opts.c nor lto-wrapper.c.  The following fixes
> > 
> > Yep, I assumed that lto-wrapper's merging is now redundant for optimization
> > options, while it is still needed for global functions (-fPIC) that was
> > not covered by my changes and since this was all bit late, it seemed I can
> > just keep it for next stage1.
> > 
> > > the target option side (for SWITCHABLE_TARGET).  Do not record
> > > any target options in the lto_opts section.
> > > 
> > > Honza - I suppose we don't have any testcase that this works, I'll
> > > try to come up with sth.  This also looks like a correctness issue
> > > to me.
> > 
> > Thanks, that would be a good idea. So with SWITCHABLE_TARGET
> > we have problem that after switching the options are misinterpretted?

Ok, so with

t.c
---
extern int have_fma;
extern double x, y, z;
void do_fma (void);
int main()
{
  if (have_fma)
    do_fma ();
  else
    x = x + y * z;
}

t2.c
---
int have_fma;
volatile double x, y, z;
void do_fma (void)
{
  x = x + y * z;
}

and doing

gcc -c -Ofast -flto t.c
gcc -c -Ofast -mfma -flto t2.c
gcc t.o t2.o

I get -mfma on the lto1 command-line for the link.  But I also get for 
main()

 <target_option_node 0x7ffff6ac40d8
    ix86_isa_flags (0x1800220000012)
    ix86_fpmath (0x2)
    target_flags (0xa0004c3)
    arch = 18 (k8)
    tune = 0 (generic)
    branch_cost = 3
    -m64 -msse2 -msse -mmmx -mfxsr -m128bit-long-double -m80387 
-mfp-ret-in-387 -mtls-direct-seg-refs -mvzeroupper 
-mavx256-split-unaligned-load -mavx256-split-unaligned-store -mfpmath=sse
>

while for do_fma()

 <target_option_node 0x7ffff6ac2e58
    ix86_isa_flags (0x117809224000112)
    ix86_fpmath (0x2)
    target_flags (0xa0004c3)
    arch = 18 (k8)
    tune = 0 (generic)
    branch_cost = 3
    -m64 -mfma -msse4.2 -msse4.1 -mssse3 -msse3 -msse2 -msse -mmmx -mfxsr 
-mpopcnt -mxsave -m128bit-long-double -m80387 -mfp-ret-in-387 
-mtls-direct-seg-refs -mvzeroupper -mavx256-split-unaligned-load 
-mavx256-split-unaligned-store -mfpmath=sse
>

and for some reason main () ends up _not_ having -mfma enabled.

I'm not sure what disables it or how we arrive at the above set of
options in the first place...  does the TARGET_OPTION_NODE store
the complete state of all target options?  I had the impression
it only stores "modification"?

So it seems to work (and simply ignore the global -mfma flag).  Not
sure on whether by design or by luck ;)

Richard.


> > I think Jakub made patch to simply skip target-option-node streaming here,
> > how this is supposed to work.
> 
> Not all targets support target attributes and I think those that
> support them are those that are SWITCHABLE_TARGET - but maybe I am
> mistaken?
> 
> > > 
> > > We can do similar changes for optimize options, now, for all targets, no?
> > 
> > I am not quite sure about this; not all options are per-function nature.
> > I need to audit all the target options for LTO safety, I did some work
> > on this for optimization options, but the target options are still very wild.
> > For example I am not quite convinced mixing -malign-double -mno-align-double units
> > will work as expected.
> 
> Well, but once types are laid out specifying sth else at link time
> won't help.  But yes, if we don't stream the option we have no chance
> to complain in lto-wrapper.  It's just that we seem to not enter
> the merging machinery for single-file testcases so I chose to "fix"
> lto option streaming rather than lto-wrapper ... I'll have a closer
> look to lto-wrapper.  But to complain about target option mismatches
> we'd have to call a target hook from lto-wrapper (or finally re-structure
> all this and merge options in lto1 itself).
> 
> Richard.
> 
> > Honza
> > > 
> > > Thanks,
> > > Richard.
> > > 
> > > 2015-04-23  Richard Biener  <rguenther@suse.de>
> > > 
> > > 	* lto-opts.c (lto_write_options): Do not record target options
> > > 	if we are targeting a SWITCHABLE_TARGET target.  We use target
> > > 	options on functions to transfer target flags from compile to
> > > 	link time.
> > > 
> > > Index: gcc/lto-opts.c
> > > ===================================================================
> > > *** gcc/lto-opts.c	(revision 222360)
> > > --- gcc/lto-opts.c	(working copy)
> > > *************** lto_write_options (void)
> > > *** 219,224 ****
> > > --- 219,230 ----
> > >   	  && lto_stream_offload_p)
> > >          continue;
> > >   
> > > +       /* Do not store target-specific options if we target a
> > > +          SWITCHABLE_TARGET target.  */
> > > +       if ((cl_options[option->opt_index].flags & CL_TARGET)
> > > + 	  && SWITCHABLE_TARGET)
> > > + 	continue;
> > > + 
> > >         /* Drop options created from the gcc driver that will be rejected
> > >   	 when passed on to the driver again.  */
> > >         if (cl_options[option->opt_index].cl_reject_driver)
> > 
> > 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild,
Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix LTO option streaming

[Jakub Jelinek]

On Thu, Apr 23, 2015 at 05:16:19PM +0200, Jan Hubicka wrote:
> > the target option side (for SWITCHABLE_TARGET).  Do not record
> > any target options in the lto_opts section.
> > 
> > Honza - I suppose we don't have any testcase that this works, I'll
> > try to come up with sth.  This also looks like a correctness issue
> > to me.
> 
> Thanks, that would be a good idea. So with SWITCHABLE_TARGET
> we have problem that after switching the options are misinterpretted?
> I think Jakub made patch to simply skip target-option-node streaming here,
> how this is supposed to work.
> > 
> > We can do similar changes for optimize options, now, for all targets, no?
> 
> I am not quite sure about this; not all options are per-function nature.
> I need to audit all the target options for LTO safety, I did some work
> on this for optimization options, but the target options are still very wild.
> For example I am not quite convinced mixing -malign-double -mno-align-double units
> will work as expected.

SWITCHABLE_TARGETs are 3, i?86/x86_64, powerpc* and mips*, but mips*
does not support target attribute (and no other targets do).

Also, not all options are allowed in target attribute, doesn't the option
need to be Target and Save, I think not all CL_TARGET options are Save.

> > Index: gcc/lto-opts.c
> > ===================================================================
> > *** gcc/lto-opts.c	(revision 222360)
> > --- gcc/lto-opts.c	(working copy)
> > *************** lto_write_options (void)
> > *** 219,224 ****
> > --- 219,230 ----
> >   	  && lto_stream_offload_p)
> >          continue;
> >   
> > +       /* Do not store target-specific options if we target a
> > +          SWITCHABLE_TARGET target.  */
> > +       if ((cl_options[option->opt_index].flags & CL_TARGET)
> > + 	  && SWITCHABLE_TARGET)
> > + 	continue;
> > + 
> >         /* Drop options created from the gcc driver that will be rejected
> >   	 when passed on to the driver again.  */
> >         if (cl_options[option->opt_index].cl_reject_driver)

	Jakub