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

[Richard Guenther <richard dot guenther at gmail dot com>]

On Fri, Oct 28, 2011 at 4:42 PM, Diego Novillo <dnovillo@google.com> wrote:
> 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.

No, not really.  I'd divide them into two classes, one where we have to
merge by union and one that we have to merge by intersection (for IL
changing options there (usually) is a conservative setting, like -fwrapv,
or -fno-strict-aliasing, or -fno-fast-math).

> 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.

For non-IL changing options things are more difficult - how should we merge
-O1 and -O2, or, -O2 -fno-tree-pre and -O2?  If there are optimization options
specified at link time we could just ignore those at compile time - but what
if there are none specified at link time?

> 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.

Or rather choose a conservative setting at link time if they differ between
units at compile-time (and of course make sure to transfer them otherwise).

>
>>
>> ! ? ? ? 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.

Basically copied from gcc.c ... (which doesn't use cl_options, thus
I can't really share code).

>> + ? /* ??? ?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.

Dropped in the followup patch (for now)

>
>> + ? 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?

Yeah, changed by using an obstack instead.

>
>
> 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.

The patch I sent as a followup (in a new thread) does that as a first step,
try to do exactly what we do now, just at the driver.

Richard.

>
> Diego.
>