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

[Zoltan Somogyi]

2020-09-28 16:01 GMT+10:00 "Peter Wang" <novalazy at gmail.com>:
>> +%---------------------%
>> +
>> +:- 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?

You are right; this code works only for handling Bool = yes.
I have added the symmetric Bool = no case as well, so that
bool cases now look like this:

       OptOption = oo_opt_delay_slot(Bool),
        OldBool = OptTuple ^ ot_opt_delay_slot,
        ( if
            getopt_io.lookup_bool_option(OptionTable, have_delay_slot, yes),
            Bool = yes
        then
            (
                OldBool = do_not_opt_delay_slot,
                !OptTuple ^ ot_opt_delay_slot := opt_delay_slot
            ;
                OldBool = opt_delay_slot
            )
        else
            (
                OldBool = do_not_opt_delay_slot
            ;
                OldBool = opt_delay_slot,
                !OptTuple ^ ot_opt_delay_slot := do_not_opt_delay_slot
            )
        )

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

I think I will fix this by making "everything in one function" a separate
boolean option, with the value of the existing procs_per_c_function
option meaningful only if this boolean does not override it. However,
I will do this in a separate change.

A related issue is: if an optimization level setting causes an integer option
to be set to e.g. 3, then the current code handles a latter setting of that option
to a higher value just fine (using int.max), but ignores any later setting of that
option to a lower value.

I will fix this by applying the max *only* to option settings implicit in -O<n>
arguments, not when a specific option is set explicitly. Again, in a separate
diff for further review.

>> +# - The first field indicates what kind of option this is. It must be
>> +#   one of "bool", "int" and "string".
> 
> or

No, the "one of" means that "and" is correct.

I followed all your other suggestions. Thanks for the review.

Zoltan.