[m-rev.] for review: optimization_options.m

[Peter Wang]

On Sun, 27 Sep 2020 12:30:16 +1000 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> This implements Mantis feature request #495. For review by Peter,
> since he made the request.
> 
> The diff does not contain a test case, since this kind of thing does not fit
> into our testing infrastructure. However, I did test it manually. An old mmc,
> when invoked with --deforestation -O2, does not perform deforestation,
> because the -O2 switches off deforestation. An mmc with this diff *does*
> perform deforestation with the same argument list.
> 
> Zoltan.
> 

> Avoid -O<n> resetting previously set options.
> 
> This implements Mantis feature request #495.
> 
...
> 
> library/getopt_template:
>     Fix a bug that screwed up an error message.
> 
>     The bug happensed when processing a --file option. If one of the

happened

>     options in the file was a special option whose special handler failed,
>     the code handling that failing option returned both an error indication,
>     and the rest of the argument list read in from the file. The code
>     handling the --file option then *ignored* the error indication from
>     the failed special option, and returned an error message of its own
>     complaining about the unconsumed remaining arguments in the file,
>     believing them to be non-option arguments, even though these arguments
>     were never looked it to see if they were options.


> diff --git a/compiler/optimization_options.m b/compiler/optimization_options.m
> index e69de29bb..fece027bf 100644
> --- a/compiler/optimization_options.m
> +++ b/compiler/optimization_options.m
> @@ -0,0 +1,1873 @@
> +%---------------------------------------------------------------------------%
> +% vim: ft=mercury ts=4 sw=4 et
> +%---------------------------------------------------------------------------%
> +% Copyright (C) 2020 The Mercury team.
> +% This file may only be copied under the terms of the GNU General
> +% Public License - see the file COPYING in the Mercury distribution.
> +%---------------------------------------------------------------------------%
> +%
> +% File: optimization_options.m
> +% Main author: zs, via tools/make_opt

I would make it clearer this file is automatically generated.

Note that git only tracks whether a file is executable or not.
Removing the write bit from a file in your workspace will prevent you
from modifying that file accidentally, but if you check out the file
again or someone else makes a fresh checkout, it will have mode 644 or 755.

> +process_optimization_options(OptionTable, OptOptions, !:OptTuple) :-
> +    !:OptTuple = init_opt_tuple,
> +    list.foldl2(update_opt_tuple(OptionTable), OptOptions,
> +        !OptTuple, not_seen_opt_level, MaybeSeenOptLevel),
> +    (
> +        MaybeSeenOptLevel = not_seen_opt_level,
> +        set_opts_upto_level(OptionTable, 0, 2,
> +            !OptTuple, MaybeSeenOptLevel, _)
> +    ;
> +        MaybeSeenOptLevel = seen_opt_level
> +    ).
> +

Maybe write "DefaultOptLevel = 2" to make it more obvious.

> +%---------------------%
> +
> +:- type maybe_seen_opt_level
> +    --->    not_seen_opt_level
> +    ;       seen_opt_level.
> +
> +:- pred update_opt_tuple(option_table::in, optimization_option::in,
> +    opt_tuple::in, opt_tuple::out,
> +    maybe_seen_opt_level::in, maybe_seen_opt_level::out) is det.
> +update_opt_tuple(OptionTable, OptOption, !OptTuple, !MaybeSeenOptLevel) :-
> +    (
> +        OptOption = oo_allow_inlining(Bool),
> +        ( if
> +            Bool = yes,
> +            !.OptTuple ^ ot_allow_inlining = do_not_allow_inlining
> +        then
> +            !OptTuple ^ ot_allow_inlining := allow_inlining
> +        else
> +            true
> +        )

Hmm, how does --no-foo work?

> +    ;
> +        OptOption = oo_procs_per_c_function(N),
> +        OldN = !.OptTuple ^ ot_procs_per_c_function,
> +        !OptTuple ^ ot_procs_per_c_function := int.max(OldN, N)

I think all the other integer values set by -O<n> increase with n,
except for this one.

-O6 implies everything_in_one_c_function by internally setting
ot_procs_per_c_function to 0.
If the user passes "--procs-per-c-function 5 -O6" then
ot_procs_per_c_function will be set to 5 instead of 0.
This is a VERY minor issue.

> +:- pred opts_enabled_at_level(int::in, list(optimization_option)::out)
> +    is semidet.
> +
> +opts_enabled_at_level(1, [
> +    % Optimization level 1: apply optimizations which are cheap and
> +    % have a good payoff while still keeping compilation time small.
> +    oo_use_local_vars(yes),
> +    oo_opt_c(yes),              % XXX We want `gcc -O1'
> +    oo_opt_frames(yes),
> +    oo_opt_delay_slot(yes),

Maybe add a comment:  % only if have_delay_slot = yes

> +    oo_opt_middle_rec(yes),
> +    oo_emit_c_loops(yes),
> +    oo_opt_mlds_tailcalls(yes)
> +]).

> diff --git a/tools/make_optimization_options_middle b/tools/make_optimization_options_middle
> index e69de29bb..ca250ea69 100755
> --- a/tools/make_optimization_options_middle
> +++ b/tools/make_optimization_options_middle
> @@ -0,0 +1,293 @@
> +#!/usr/bin/awk -f
> +# vim: ts=4 sw=4 et ft=awk
> +#---------------------------------------------------------------------------#
> +# Copyright (C) 2020 The Mercury team.
> +# This file may only be copied under the terms of the GNU General
> +# Public License - see the file COPYING in the Mercury distribution.
> +#---------------------------------------------------------------------------#
> +#
> +# The input to this file, make_optimization_options_db, has one line
> +# for every optimization option. Each line must have either three
> +# or four fields.
> +#
> +# - The first field indicates what kind of option this is. It must be
> +#   one of "bool", "int" and "string".

or

> +#
> +# - The second field gives the initial value of the option.
> +#
> +#   - If the option is boolean, the initial value must be either "n" or "y".
> +#
> +#   - If the option is an integer, the initial value must be an integer.
> +#
> +#   - If the option is a strin, the initial value will be set
> +#     to the empty string regardless of the contents of the second field.

string

> +#
> +# - The third field is the name of the option in options.m.
> +#
> +# - The fourth field, if it exists, gives the name of the option
> +#   in the file this script helps generate, optimization_options.m.
> +#   If there is no fourth field, the name is taken to be the same
> +#   as the name in the third field.
> +#
> +#   For boolean options, the name in optimization_options.m should be a verb,
> +#   because when we generate a bespoke type for it, its two function symbols
> +#   will be named "verb" and "do_not_verb".
> +#
> +
> +BEGIN {
> +        default_opt_level = 2;
> +        next_bool_opt = 0;
> +        next_int_opt = 0;
> +        next_string_opt = 0;
> +    }
> +    {
> +        if (NF == 3 || NF == 4) {

You could assign the fields to variables, and handle the optional fourth
field here instead of each case.

> +            if ($1 == "bool") {
> +                bool_opts[next_bool_opt] = $3;
> +                if (NF == 4) {
> +                    bool_field_names[next_bool_opt] = $4;
> +                } else {
> +                    bool_field_names[next_bool_opt] = $3;
> +                }
> +                if ($2 == "y") {
> +                    bool_defaults[next_bool_opt] = bool_field_names[next_bool_opt];
> +                } else if ($2 == "n") {
> +                    bool_defaults[next_bool_opt] = "do_not_" bool_field_names[next_bool_opt];
> +                } else {
> +                    printf("ERROR: bad bool default<%s>\n", $0);
> +                }
> +                next_bool_opt++;
> +            } else if ($1 == "int") {
> +                int_opts[next_int_opt] = $3;
> +                if (NF == 4) {
> +                    int_field_names[next_int_opt] = $4;
> +                } else {
> +                    int_field_names[next_int_opt] = $3;
> +                }
> +                int_defaults[next_int_opt] = $2 + 0;
> +                next_int_opt++;
> +            } else if ($1 == "string") {
> +                string_opts[next_string_opt] = $3;
> +                if (NF == 4) {
> +                    string_field_names[next_string_opt] = $4;
> +                } else {
> +                    string_field_names[next_string_opt] = $3;
> +                }
> +                # Ignore the default; strings always default to the empty string.
> +                next_string_opt++;

Might as well require the second field to be "-" for now.

> +            } else {
> +                printf("ERROR: line with unknown type: <%s>\n", $0);
> +            }
> +        } else {
> +            printf("ERROR: line with %d fields: <%s>\n", NF, $0);
> +        }
> +    }

The rest looks fine. Thanks for working on it!

Peter