for tryout and/or review: removing globals from the I/O state

[Zoltan Somogyi-2]

I would like feedback on this diff mostly in the form of "mmc --make" users
trying out a version of the compiler with this diff, which I have installed
on goliath in /home/goliath/workspaces/zs/test_install/bin. mmc --make
is the functionality that is probably most affected by this diff. The diff
has passed bootchecks in several grades, but bootcheck's tests of mmc --make
are not as tough as real life use. I would also appreciate reviews, but
the diff is 54,000+ lines long, and most of it is very boring. It is also
more than 2 Mb in size, above the mail system's size limit, so I put it
in a file for any reviewers to look at: ~zs/tmp/DIFF.globals_iostate.
The diff still contains ZZZs marking the spots that at one time or another
I thought deserved a second look. I intend to remove these ZZZs before commit,
since I think I later made sure they were handled correctly, but any reviewers
will probably want to concentrate on those spots.

I would like to commit this as soon as possible, since the size of the diff
makes conflicts between it and any other change to the compiler quite likely.
(I already had to resolve a few.) On the other hand, I don't want to commit
a diff that breaks mmc --make for people. That means I would really appreciate
prompt feedback.

Zoltan.

-------------------------------------------------------------------------

Stop storing globals in the I/O state, and divide mercury_compile.m
into smaller, more cohesive modules. (This diff started out as doing
only the latter, but it became clear that this was effectively impossible
without the former, and the former ended up accounting for the bulk of the
changes.)

Taking the globals out of the I/O state required figuring out how globals
data flowed between pieces of code that were often widely separated.
Such flows were invisible when globals could be hidden in the I/O state,
but now they are visible, because the affected code now passes around
globals structures explicitly.

In some cases, the old flow looked buggy, as when one job invoked by
mmc --make could affect the globals value of its parent or the globals value
passed to the next job. I tried to fix such problems when I saw them. I am
not 100% sure I succeeded in every case (I may have replaced old bugs with
new ones), but at least now the flow is out in the open, and any bugs
should be much easier to track down and fix.

In most cases, changes the globals after the initial setup are intended to be
in effect only during the invocation of a few calls. This used to be done
by remembering the initial values of the to-be-changed options, changing their
values in the globals in the I/O state, making the calls, and restoring the old
values of the options. We now simply create a new version of the globals
structure, pass it to the calls to be affected, and then discard it.

In two cases, when discovering reasons why (1) smart recompilation should
not be done or (2) item version numbers should not be generated, the record
of the discovery needs to survive this discarding. This is why in those cases,
we record the discovery by setting a mutable attached to the I/O state.
We use pure code (with I/O states) both to read and to write the mutables,
so this is no worse semantically than storing the information in the globals
structure inside the I/O state. (Also, we were already using such a mutable
for recording whether -E could add more information.)

In many modules, the globals information had to be threaded through
several predicates in the module. In some places, this was made more
difficult by predicates being defined by many clauses. In those cases,
this diff converts those predicates to using explicit disjunctions.

compiler/globals.m:
        Stop storing the globals structure in the I/O state, and remove
        the predicates that accessed it there.

        Move a mutable and its access predicate here from handle_options.m,
        since here is when the mutables treated the same way are.

        In a couple of cases, the value of an option is available in a mutable
        for speed of access from inside performance-critical code. Set the
        values of those mutables from the option when the processing of option
        values is finished, not when it is starting, since otherwise the copies
        of each option could end up inconsistent.

        Validate the reuse strategy option here, since doing it during ctgc
        analysis (a) is too late, and (b) would require an update to the
        globals to be done at an otherwise inconvenient place in the code.
        Put the reuse strategy into the globals structure.

        Two fields in the globals structure were unused. One
        (have_printed_usage) was made redundant when the one predicate
        that used it itself became unused; the other (source_file_map)
        was effectively replaced by a mutable some time ago. Delete
        these fields from the globals.

        Give the fields of the globals structure a distinguishing prefix.

        Put the type declarations, predicate declarations and predicate
        definitions in a consistent order.

compiler/source_file_map.m:
        Record this module's results only in the mutable (it serves as a
        cache), not in globals structure. Use explicitly passed globals
        structure for other purposes.

compiler/handle_options.m:
        Rename handle_options as handle_given_options, since it does not
        process THE options to the program, but the options it is given,
        and even during the processing of a single module, it can be invoked
        up the three times in a row, each time being given different options.
        (It was up to four times in a row before this diff.)

        Make handle_given_options explicitly return the globals structure it
        creates. Since it does not take an old global structure as input
        and globals are not stored in the I/O state, it is now clear that
        the globals structure it returns is affected only by the default values
        of the options and the options it processes. Before this diff,
        in the presence of errors in the options, handle_options *could*
        return (implicitly, in the I/O state) the globals structure that
        happened to be in the I/O state when it was invoked.

        Provide a separate predicate for generating a dummy globals based only
        on the default values of options. This allows by mercury_compile.m
        to stop abusing a more general-purpose predicate from handle_options.m,
        which we no longer export.

        Remove the mutable and access predicate moved to globals.m.

compiler/options.m:
        Document the fact that two options, smart_recompilation and
        generate_item_version_numbers, should not be used without seeing
        whether the functionalities they call for have been disabled.

compiler/mercury_compile_front_end.m:
compiler/mercury_compile_middle_passes.m:
compiler/mercury_compile_llds_back_end.m:
compiler/mercury_compile_mlds_back_end.m:
compiler/mercury_compile_erl_back_end.m:
        New modules carved out of the old mercury_compile.m. They each cover
        exactly the areas suggested by their names.

        Each of the modules is more cohesive than the old mercury_compile.m.
        Their code is also arranged in a more logical order, with predicates
        representing compiler passes being defined in the order of their
        invocation.

        Some of these modules export predicates for use by their siblings,

compiler/top_level.m:
compiler/notes/compiler_design.html:
        Add the new modules.

compiler/mark_static_terms.m:
        Move this module from the ml_backend package to the hlds package,
        since (a) it does not depend on the MLDS in any way, and (b) it is
        also needed by a compiler pass (loop invariants) in the middle passes.

compiler/hlds.m:
compiler/ml_backend.m:
compiler/notes/compiler_design.html:
        Reflect mark_static_terms.m's change of package.

compiler/passes_aux.m:
        Move the predicates for dumping out the hLDS here from
        mercury_compile.m, since the new modules also need them.

        Look up globals in the HLDS, not the I/O state.

compiler/hlds_module.m:
        Store the prefix (common part) of HLDS dump file names in the HLDS
        itself, so that the code moved to passes_aux.m can figure out the
        file name for a HLDS dump without doing system calls.

        Give the field names of some structures prefixes to avoid ambiguity.

compiler/mercury_compile.m:
        Remove the code moved to the other modules. This module now looks
        after only option handling (such as deciding whether to generate .int3
        files, .int files, .opt files etc), and the compilation passes
        up to and including the creation of the first version of the HLDS.
        Everything after that is subcontracted to the new modules.

        Simplify and make explicit the flow of globals information.
        When invoking predicates that could disable smart recompilation,
        check whether they have done so, and if yes, update the globals
        accordingly.

        When compiling via gcc, we need to link into the executable
        the object files of any separate C files we generate for C code
        foreign_procs, which we cannot translate into gcc's internal
        structures without becoming a C compiler as well as a Mercury compiler.
        Instead of adding such files to the accumulating option for extra
        object files in the globals structure, we return their names using
        the already existing mechanism we have always used to link the object
        files of fact tables into the executable.

        Give several predicates more descriptive names. Put predicates
        in a more logical order.

compiler/make.m:
compiler/make.dependencies.m:
compiler/make.module_target.m:
compiler/make.module_dep_file.m:
compiler/make.program_target.m:
compiler/make.util.m:
        Require callers to supply globals structures explicitly, not via the
        I/O state. Afterward pass them around explicitly, passing modified
        versions to mercury_compile.m when invoking it with module- and/or
        task-specific options.

        Due the extensive use of partial application for higher order code
        in these modules, passing around the globals structures explicitly
        is quite tricky here. There may be cases where a predicate uses
        an old globals structure it got from a closure instead of the updated
        module- and/or task-specific globals it should be using, or vice versa.
        However, it is just as likely that, this diff fixes old problems
        by preventing the implicit flow of updated-only-for-one-invocation
        globals structures back to the original invoking context.

        Although I have tried to be careful about this, it is also possible
        that in some places, the code is using an updated-for-an-invocation
        globals structure in some but not all of the places where it
        SHOULD be used.

compiler/c_util.m:
compiler/compile_target_code.m:
compiler/compiler_util.m:
compiler/error_util.m:
compiler/file_names.m:
compiler/file_util.m:
compiler/ilasm.m:
compiler/ml_optimize.m:
compiler/mlds_to_managed.m:
compiler/module_cmds.m:
compiler/modules.m:
compiler/options_file.m:
compiler/pd_debug.m:
compiler/prog_io.m:
compiler/transform_llds.m:
compiler/write_deps_file.m:
        Require callers to supply globals structures explicitly, not via the
        I/O state.

        In some cases, the explicit globals structure argument allows
        a predicate to dispense with the I/O states previously passed to it.

        In some modules, rename some predicates, types and/or function symbols
        to avoid ambiguity.

compiler/read_modules.m:
        Require callers to supply globals structures explicitly, not via the
        I/O state.

        Record when smart recompilation and the generation of item version
        numbers should be disabled.

compiler/opt_debug.m:
compiler/process_util.m:
        Require callers to supply the needed options explicitly, not via the
        globals in the I/O state.

compiler/analysis.m:
compiler/analysis.file.m:
compiler/mmc_analysis.m:
        Make the analysis framework's methods take their global structures
        as explicit arguments, not as implicit data stored in the I/O state.

        Stop using `with_type` and `with_inst` declarations unnecessarily.

        Rename some predicates to avoid ambiguity.

compiler/hlds_out.m:
compiler/llds_out.m:
compiler/mercury_to_mercury.m:
compiler/mlds_to_c.m:
compiler/mlds_to_java.m:
compiler/optimize.m:
        Make these modules stop accessing the globals from the I/O state.
        Do this by requiring the callers of their top predicates to explicitly
        supply a globals structure. To compensate for the cost of having to
        pass around a representation of the options, look up the values of the
        options of interest just once, to make further access much faster.

        (In the case of mlds_to_c.m, the code already did much of this,
        but it still had a few accesses to globals in the I/O state that
        this diff eliminates.)

        If the module exports a predicate that needs these pre-looked-up
        options, then export the type of this data structure and its
        initialization function.

compiler/frameopt.m:
        Since this module needs only one option from the globals, pass that
        option instead of the globals.

compiler/accumulator.m:
compiler/add_clause.m:
compiler/closure_analysis.m:
compiler/complexity.m:
compiler/deforest.m:
compiler/delay_construct.m:
compiler/elds_to_erlang.m:
compiler/exception_analysis.m:
compiler/fact_table.m:
compiler/intermod.m:
compiler/mode_constraints.m:
compiler/mode_errors.m:
compiler/pd_util.m:
compiler/post_term_analysis.m:
compiler/recompilation.usage.m:
compiler/size_prof.usage.m:
compiler/structure_reuse.analysis.m:
compiler/structure_reuse.direct.choose_reuse.m:
compiler/structure_reuse.direct.m:
compiler/structure_sharing.analysis.m:
compiler/tabling_analysis.m:
compiler/term_constr_errors.m:
compiler/term_constr_fixpoint.m:
compiler/term_constr_initial.m:
compiler/term_constr_main.m:
compiler/term_constr_util.m:
compiler/trailing_analysis.m:
compiler/trans_opt.m:
compiler/typecheck_info.m:
        Look up globals information from the HLDS, not the I/O state.

        Conform to the changes above.

compiler/gcc.m:
compiler/maybe_mlds_to_gcc.pp:
compiler/mlds_to_gcc.m:
        Look up globals information from the HLDS, not the I/O state.

        Conform to the changes above.

        Convert these modules to our current programming style.

compiler/termination.m:
        Look up globals information from the HLDS, not the I/O state.

        Conform to the changes above.

        Report some warnings with error_specs, instead of immediately
        printing them out.

compiler/export.m:
compiler/il_peephole.m:
compiler/layout_out.m:
compiler/rtti_out.m:
compiler/liveness.m:
compiler/make_hlds.m:
compiler/make_hlds_passes.m:
compiler/mlds_to_il.m:
compiler/mlds_to_ilasm.m:
compiler/recompilation.check.m:
compiler/stack_opt.m:
compiler/superhomogeneous.m:
compiler/tupling..m:
compiler/unneeded_code.m:
compiler/unused_args.m:
compiler/unused_import.m:
compiler/xml_documentation.m:
        Conform to the changes above.

compiler/equiv_type_hlds.m:
        Give the field names of a structure prefixes to avoid ambiguity.

        Stop using `with_type` and `with_inst` declarations unnecessarily.

compiler/loop_inv.m:
compiler/pd_info.m:
compiler/stack_layout.m:
        Give the field names of some structures prefixes to avoid ambiguity.

compiler/add_pragma.m:
        Add notes.

compiler/string.m:
        Add a det version of remove_suffix, for use by new code above.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews@...
Administrative Queries: owner-mercury-reviews@...
Subscriptions:          mercury-reviews-request@...
--------------------------------------------------------------------------

[Julien Fischer-2]

On Wed, 14 Oct 2009, Zoltan Somogyi wrote:

> I would like feedback on this diff mostly in the form of "mmc --make" users
> trying out a version of the compiler with this diff, which I have installed
> on goliath in /home/goliath/workspaces/zs/test_install/bin. mmc --make
> is the functionality that is probably most affected by this diff. The diff
> has passed bootchecks in several grades, but bootcheck's tests of mmc --make
> are not as tough as real life use. I would also appreciate reviews, but
> the diff is 54,000+ lines long, and most of it is very boring. It is also
> more than 2 Mb in size, above the mail system's size limit, so I put it
> in a file for any reviewers to look at: ~zs/tmp/DIFF.globals_iostate.
> The diff still contains ZZZs marking the spots that at one time or another
> I thought deserved a second look. I intend to remove these ZZZs before commit,
> since I think I later made sure they were handled correctly, but any reviewers
> will probably want to concentrate on those spots.
>
> I would like to commit this as soon as possible, since the size of the diff
> makes conflicts between it and any other change to the compiler quite likely.
> (I already had to resolve a few.) On the other hand, I don't want to commit
> a diff that breaks mmc --make for people. That means I would really appreciate
> prompt feedback.

I have compiled G12 with the above compiler (both with and without
--enable-opt-high**) and also run all the regression tests.  I found
no problem with this change and mmc --make.

** --intermodule-optimization -O5 --optimize-constructor-last-call
    --common-struct

Julien.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews@...
Administrative Queries: owner-mercury-reviews@...
Subscriptions:          mercury-reviews-request@...
--------------------------------------------------------------------------

[Ian MacLarty-4]

On Wed, Oct 14, 2009 at 1:29 PM, Zoltan Somogyi <zs@...> wrote:

> I would like feedback on this diff mostly in the form of "mmc --make" users
> trying out a version of the compiler with this diff, which I have installed
> on goliath in /home/goliath/workspaces/zs/test_install/bin. mmc --make
> is the functionality that is probably most affected by this diff. The diff
> has passed bootchecks in several grades, but bootcheck's tests of mmc --make
> are not as tough as real life use. I would also appreciate reviews, but
> the diff is 54,000+ lines long, and most of it is very boring. It is also
> more than 2 Mb in size, above the mail system's size limit, so I put it
> in a file for any reviewers to look at: ~zs/tmp/DIFF.globals_iostate.
> The diff still contains ZZZs marking the spots that at one time or another
> I thought deserved a second look. I intend to remove these ZZZs before commit,
> since I think I later made sure they were handled correctly, but any reviewers
> will probably want to concentrate on those spots.
>
> I would like to commit this as soon as possible, since the size of the diff
> makes conflicts between it and any other change to the compiler quite likely.
> (I already had to resolve a few.) On the other hand, I don't want to commit
> a diff that breaks mmc --make for people. That means I would really appreciate
> prompt feedback.
>

I tried this change on one of the MC projects that use --make and it
worked fine.

Ian.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews@...
Administrative Queries: owner-mercury-reviews@...
Subscriptions:          mercury-reviews-request@...
--------------------------------------------------------------------------

[Peter Wang-6]

On 2009-10-14, Zoltan Somogyi <zs@...> wrote:

> compiler/mercury_compile_front_end.m:
> compiler/mercury_compile_middle_passes.m:
> compiler/mercury_compile_llds_back_end.m:
> compiler/mercury_compile_mlds_back_end.m:
> compiler/mercury_compile_erl_back_end.m:
> New modules carved out of the old mercury_compile.m. They each cover
> exactly the areas suggested by their names.
>
> Each of the modules is more cohesive than the old mercury_compile.m.
> Their code is also arranged in a more logical order, with predicates
> representing compiler passes being defined in the order of their
> invocation.
>
> Some of these modules export predicates for use by their siblings,

... ?

> compiler/string.m:
> Add a det version of remove_suffix, for use by new code above.

Update NEWS.

--- compiler/make.m     14 Aug 2009 20:37:46 -0000      1.60
+++ compiler/make.m     12 Oct 2009 06:01:38 -0000
@@ -609,8 +608,9 @@                                                                      
     io::di, io::uo) is det.                                                            
                                                                                         
 option_table_hash(AllOptionArgs, Hash, !IO) :-                                          
-    globals.io_get_globals(Globals, !IO),                                              
-    handle_options(AllOptionArgs, OptionsErrors, _, _, _, !IO),                        
+    handle_given_options(AllOptionArgs, _, _, _,                                        
+        OptionsErrors, UpdatedGlobals, !IO),                                            
+    % XXX We throw away UpdatedGlobals after computing its hash. Why? -zs              
                                                                                         
We have no other use for the updated globals here.

--- compiler/mercury_compile.m  25 Sep 2009 05:13:02 -0000      1.505
+++ compiler/mercury_compile.m  12 Oct 2009 06:23:54 -0000
...
+        % (make.module_target does this to overcome limits on the lengths
+        % of command lines on Windows.) The environment is ignored, unlike
+        % with @file syntax.
+
+        % XXX I think the presence of this assignment here means that
+        % `mmc --mmake' does not use this mechanism for linking, but I am
+        % not sure. -zs
+        Link = no,

That's right.

Peter
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews@...
Administrative Queries: owner-mercury-reviews@...
Subscriptions:          mercury-reviews-request@...
--------------------------------------------------------------------------

[Zoltan Somogyi-2]

On 14-Oct-2009, Peter Wang <novalazy@...> wrote:
> > Add a det version of remove_suffix, for use by new code above.
>
> Update NEWS.

Will do.

> --- compiler/make.m     14 Aug 2009 20:37:46 -0000      1.60
> +++ compiler/make.m     12 Oct 2009 06:01:38 -0000
> @@ -609,8 +608,9 @@
>  option_table_hash(AllOptionArgs, Hash, !IO) :-
> -    globals.io_get_globals(Globals, !IO),
> -    handle_options(AllOptionArgs, OptionsErrors, _, _, _, !IO),
> +    handle_given_options(AllOptionArgs, _, _, _,
> +        OptionsErrors, UpdatedGlobals, !IO),
> +    % XXX We throw away UpdatedGlobals after computing its hash. Why? -zs
>
> We have no other use for the updated globals here.

I figured that, but in that case, why hash the updated globals and not the
original ones? Clearly, the updated globals are being hashed because they
will be or have been used, in which case, why do the update twice?

> +        % XXX I think the presence of this assignment here means that
> +        % `mmc --mmake' does not use this mechanism for linking, but I am
> +        % not sure. -zs
> +        Link = no,
>
> That's right.

Will update the comment.

Thanks for all the tryouts and reviews.

I have committed the diff. Since the diff changes the package to which
mark_static_terms belongs, you will need to remove up old, now misnamed files
(ml_backend.mark_static_terms.*) An "mmake clean" will do this if you
invoke it BEFORE you invoke "mmake depend", but not after; after, you will
need to remove them manually.

Zoltan.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews@...
Administrative Queries: owner-mercury-reviews@...
Subscriptions:          mercury-reviews-request@...
--------------------------------------------------------------------------

[Peter Wang-6]

On 2009-10-14, Zoltan Somogyi <zs@...> wrote:

>
> > --- compiler/make.m     14 Aug 2009 20:37:46 -0000      1.60
> > +++ compiler/make.m     12 Oct 2009 06:01:38 -0000
> > @@ -609,8 +608,9 @@
> >  option_table_hash(AllOptionArgs, Hash, !IO) :-
> > -    globals.io_get_globals(Globals, !IO),
> > -    handle_options(AllOptionArgs, OptionsErrors, _, _, _, !IO),
> > +    handle_given_options(AllOptionArgs, _, _, _,
> > +        OptionsErrors, UpdatedGlobals, !IO),
> > +    % XXX We throw away UpdatedGlobals after computing its hash. Why? -zs
> >
> > We have no other use for the updated globals here.
>
> I figured that, but in that case, why hash the updated globals and not the
> original ones? Clearly, the updated globals are being hashed because they
> will be or have been used, in which case, why do the update twice?

This code is part of the --track-flags implementation.  We hash the
updated globals[*] because they include module-specific options.  The
hash is then compared with the hash in a MODULE.track_flags file, which
is updated if it differs.  The new timestamp will later force the module
to be recompiled if necessary, but that's later.  We aren't compiling
the module immediately so the updated globals have no more use here.

Peter

[*] actually the options.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews@...
Administrative Queries: owner-mercury-reviews@...
Subscriptions:          mercury-reviews-request@...
--------------------------------------------------------------------------

[Zoltan Somogyi-2]

On 14-Oct-2009, Peter Wang <novalazy@...> wrote:
> This code is part of the --track-flags implementation.  We hash the
> updated globals[*] because they include module-specific options.  The
> hash is then compared with the hash in a MODULE.track_flags file, which
> is updated if it differs.  The new timestamp will later force the module
> to be recompiled if necessary, but that's later.  We aren't compiling
> the module immediately so the updated globals have no more use here.

Thanks for that. I updated the comment.

Zoltan.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews@...
Administrative Queries: owner-mercury-reviews@...
Subscriptions:          mercury-reviews-request@...
--------------------------------------------------------------------------

[Julien Fischer-2]

On Wed, 14 Oct 2009, Julien Fischer wrote:

> On Wed, 14 Oct 2009, Zoltan Somogyi wrote:
>
>> I would like feedback on this diff mostly in the form of "mmc --make" users
>> trying out a version of the compiler with this diff, which I have installed
>> on goliath in /home/goliath/workspaces/zs/test_install/bin. mmc --make
>> is the functionality that is probably most affected by this diff. The diff
>> has passed bootchecks in several grades, but bootcheck's tests of mmc
>> --make
>> are not as tough as real life use. I would also appreciate reviews, but
>> the diff is 54,000+ lines long, and most of it is very boring. It is also
>> more than 2 Mb in size, above the mail system's size limit, so I put it
>> in a file for any reviewers to look at: ~zs/tmp/DIFF.globals_iostate.
>> The diff still contains ZZZs marking the spots that at one time or another
>> I thought deserved a second look. I intend to remove these ZZZs before
>> commit,
>> since I think I later made sure they were handled correctly, but any
>> reviewers
>> will probably want to concentrate on those spots.
>>
>> I would like to commit this as soon as possible, since the size of the diff
>> makes conflicts between it and any other change to the compiler quite
>> likely.
>> (I already had to resolve a few.) On the other hand, I don't want to commit
>> a diff that breaks mmc --make for people. That means I would really
>> appreciate
>> prompt feedback.
>
> I have compiled G12 with the above compiler (both with and without
> --enable-opt-high**) and also run all the regression tests.  I found
> no problem with this change and mmc --make.

I have also tried bootchecking the compiler with --use-mmc-make enabled,
and did not find any problems with this change either.

Julien.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews@...
Administrative Queries: owner-mercury-reviews@...
Subscriptions:          mercury-reviews-request@...
--------------------------------------------------------------------------