Re: [PATCH][RFC] Re-write LTO option merging

[Diego Novillo <dnovillo at google dot com>]

On 11-10-27 01:46 , Richard Guenther wrote:
> On Wed, 26 Oct 2011, Richard Guenther wrote:
>
> > This completely rewrites LTO option merging.  At compile (uselessly
> > now at WPA?) time we now stream a COLLECT_GCC_OPTIONS like string
> > as it comes from argv of the compiler binary.  Those options are
> > read in by the LTO driver (lto-wrapper), merged into a single
> > set (very simple merge function right now ;)) and given a place to
> > complain about incompatible arguments.  The merged set is then
> > prepended to the arguments from the linker driver line
> > (what we get in COLLECT_GCC_OPTIONS for lto-wrapper), thus the
> > linker command-line may override what the compiler command-line(s)
> > provided.
> >
> > One visible change is that no optimization option on the link line
> > no longer means -O0, unless you explicitly specify -O0 at link time.
> >
> > There are probably more obscure differences, especially due to the
> > very simple merge and complain function ;))  But this is a RFC ...
> >
> > If WPA partitioning at any point wants to do something clever with
> > a set of incompatible functions it can re-parse the options and
> > do that (we then have to arrange for lto-wrapper to let the options
> > slip through).
> >
> > I'm LTO bootstrapping and testing this simple variant right now
> > (I believe we do not excercise funny option combinations right now).
> >
> > I'll still implement a very simple merge/complain function.
> > Suggestions for that welcome (I'll probably simply compute the
> > intersection of options ... in the long run we'd want to annotate
> > our options as to whether they should be unioned/intersected).

Are you thinking of having some table of options with hints?  An NxN 
matrix of options?  Given two arbitrary options OPT1 and OPT2, how do we 
decide whether they can go together?  That's one big matrix.

Perhaps we could group options in classes?  There's really only a subset 
of options that need to be checked: -f, -m, -O, -g, ...
Perhaps start with an if-tree checking an incoming option against the 
set of accumulated options so far.

In fact, if we simply cataloged the set of options that can affect 
gimple bytecode generation, we can then make sure that those don't 
change at link time.


> !       if (i != 1)
> ! 	obstack_grow (&temporary_obstack, " ", 1);
> !       obstack_grow (&temporary_obstack, "'", 1);
> !       q = option->canonical_option[0];
> !       while ((p = strchr (q, '\'')))
> ! 	{
> ! 	  obstack_grow (&temporary_obstack, q, p - q);
> ! 	  obstack_grow (&temporary_obstack, "'\\''", 4);
> ! 	  q = ++p;
> ! 	}
> !       obstack_grow (&temporary_obstack, q, strlen (q));
> !       obstack_grow (&temporary_obstack, "'", 1);
>
> !       for (j = 1; j<  option->canonical_option_num_elements; ++j)
>    	{
> ! 	  obstack_grow (&temporary_obstack, " '", 2);
> ! 	  q = option->canonical_option[j];
> ! 	  while ((p = strchr (q, '\'')))
> ! 	    {
> ! 	      obstack_grow (&temporary_obstack, q, p - q);
> ! 	      obstack_grow (&temporary_obstack, "'\\''", 4);
> ! 	      q = ++p;
> ! 	    }
> ! 	  obstack_grow (&temporary_obstack, q, strlen (q));
> ! 	  obstack_grow (&temporary_obstack, "'", 1);

Ugh.

> +   /* ???  For now the easiest thing would be to warn about
> +      mismatches.  */
> +
> +   if (*decoded_options_count != fdecoded_options_count)
> +     {
> +       /* ???  Warn?  */
> +       return;
> +     }

Yes, please.  We don't want to silently accept anything we don't fully 
understand.


> +   for (i = 0; i<  *decoded_options_count; ++i)
> +     {
> +       struct cl_decoded_option *option =&(*decoded_options)[i];
> +       struct cl_decoded_option *foption =&fdecoded_options[i];
> +       if (strcmp (option->orig_option_with_args_text,
> + 		  foption->orig_option_with_args_text) != 0)
> + 	{
> + 	  /* ???  Warn?  */
> + 	  return;

Likewise.  If the warning proves to noisy in common scenarios, we can 
then adjust.



> +
>      /* Initalize the common arguments for the driver.  */
> !   new_argv = (const char **) xmalloc ((15

15?



One thing I like about this is that it moves option processing out of 
lto1.  If we are getting the same behaviour as today, I'd say commit and 
we can refine later.


Diego.