[m-rev.] for review: set optimization options just once

Julien Fischer jfischer at opturion.com
Sun Oct 11 15:45:41 AEDT 2020


Hi Zoltan,

Thanks for that!

On Sun, 11 Oct 2020, Zoltan Somogyi wrote:

> For review by Julien, since it is an attempt to fix a bug he reported,
> Mantis 522. The optimization improvement mentioned in the log message
> has bootchecked, but I haven't tried it out on this code yet.

I will try it out after you commit.

> Update each optimization option just once.
> 
> compiler/handle_options.m:
>     The huge predicate that handles the implications among options
>     used to have many places that set the values of optimization options.
>     When each of these places just called global.set_option, this was fine.
>     Unfortunately, when the switch to the use of the opt_tuple replaced
>     each such call with a field update on a tuple with *many* fields,
>     the result was that the Java code we generated for this huge predicate
>     exceeded the JVM's limit on the size of a single method.
>
>     This diff moves all the updates of the opt_tuple to one block of code
>     at the end of the predicate. This *should* allow the compiler
>     to replace all the updated fields of the opt_tuple at once,
>     though right now, the optimization that is supposed to do that
>     is not up to the job.
>
>     However, this change is worth while regardless of the impact on Java.
>     In the old code, a bunch of options had their values set in several
>     places each. *Most* of the time, the settings were consistent,
>     such as turning off an optimization for this or that reason.
>     However, in some places, one piece of code explicitly turned
>     an option off, and then a later piece of code explicitly turned it
>     back on again. The new approach detects such inconsistencies,
>     and forces them to be resolved. I added XXXs to record the previous
>     presence of such inconsistencies, in case some Mercury maintainer
>     (or software archaeologist) wants to know such things in the future.

...

> diff --git a/compiler/handle_options.m b/compiler/handle_options.m
> index 42d9672..10d80fe 100644
> --- a/compiler/handle_options.m
> +++ b/compiler/handle_options.m

...

> @@ -1596,18 +1658,26 @@ convert_options_to_globals(OptionTable0, !.OptTuple, OpMode, Target, GC_Method,
>                  words("high level code."), nl],
>              add_error(phase_options, DeepHLSpec, !Specs)
>          ),
> -        !OptTuple ^ ot_opt_lcmc := do_not_opt_lcmc,
> +        AllowOptLCMCProfDeep = bool.no,
>          globals.lookup_bool_option(!.Globals,
>              use_lots_of_ho_specialization, LotsOfHOSpec),
>          (
> -            LotsOfHOSpec = yes,
> -            !OptTuple ^ ot_opt_higher_order := opt_higher_order,
> -            !OptTuple ^ ot_higher_order_size_limit := 999999
> +            LotsOfHOSpec = bool.yes,
> +            % XXX We used to switch on --optimize-higher-order here,
> +            % even if it was disabled e.g. for execution tracing.
> +            % That seems wrong.

It was probably wrong but nobody noticed.  The main use case for lots of ho
specialization was deep profiling which is rarely used in conjunction with
execution tracing.

> +            %
> +            % Setting the limit here is ok, since the limit is irrelevant
> +            % if the only optimization that looks at it is not run.
> +            OT_HigherOrderSizeLimit = 999999
>          ;
> -            LotsOfHOSpec = no
> +            LotsOfHOSpec = bool.no,
> +            OT_HigherOrderSizeLimit = OT_HigherOrderSizeLimit0
>          )
>      ;
> -        ProfileDeep = no
> +        ProfileDeep = bool.no,
> +        OT_HigherOrderSizeLimit = OT_HigherOrderSizeLimit0,
> +        AllowOptLCMCProfDeep = bool.yes
>      ),
>
>      globals.lookup_bool_option(!.Globals, record_term_sizes_as_words,

...

> @@ -1704,28 +1812,40 @@ convert_options_to_globals(OptionTable0, !.OptTuple, OpMode, Target, GC_Method,
>      % `--no-optimize-constant-propagation' otherwise,
>      % e.g. when tracing is enabled.
>      (
> -        ProfileDeep = no,
> -        AllowInlining = !.OptTuple ^ ot_allow_inlining,
> +        ProfileDeep = bool.no,
>          (
> -            AllowInlining = do_not_allow_inlining,
> -            !OptTuple ^ ot_prop_constants := do_not_prop_constants
> +            OT_AllowInlining = do_not_allow_inlining,
> +            AllowPropConstantsProfDeep = bool.no
>          ;
> -            AllowInlining = allow_inlining
> +            OT_AllowInlining = allow_inlining,
> +            AllowPropConstantsProfDeep = bool.yes
>          )
>      ;
> -        ProfileDeep = yes
> +        ProfileDeep = bool.yes,
> +        AllowPropConstantsProfDeep = bool.yes
> +    ),
> +    ( if
> +        AllowPropConstantsInlineBuiltins = bool.yes,
> +        AllowPropConstantsProfDeep = bool.yes
> +    then
> +        OT_PropConstants = OT_PropConstants0
> +    else
> +        OT_PropConstants = do_not_prop_constants
>      ),
>
>      % --no-reorder-conj implies --no-deforestation,
>      % --no-constraint-propagation and --no-local-constraint-propagation.
>      globals.lookup_bool_option(!.Globals, reorder_conj, ReorderConj),
>      (
> -        ReorderConj = no,
> -        !OptTuple ^ ot_deforest := do_not_deforest,
> -        !OptTuple ^ ot_prop_constraints := do_not_prop_constraints,
> -        !OptTuple ^ ot_prop_local_constraints := do_not_prop_local_constraints
> +        ReorderConj = bool.no,
> +        AllowDeforestReorderConj = bool.no,
> +        % YYY AllowPropConstraintsReorderConj = bool.no,
> +        AllowPropLocalConstraintsReorderConj = bool.no
>      ;
> -        ReorderConj = yes
> +        ReorderConj = bool.yes,
> +        AllowDeforestReorderConj = bool.yes,
> +        % YYY AllowPropConstraintsReorderConj = bool.yes
> +        AllowPropLocalConstraintsReorderConj = bool.yes
>      ),

What's with the YYY comments?

The rest looks fine.

Julien.


More information about the reviews mailing list