[m-rev.] for review: Add --trans-opt-deps-spec option.

[Peter Wang]

On Thu, 12 Jan 2023 15:14:59 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> 2023-01-12 14:13 GMT+11:00 "Peter Wang" <novalazy at gmail.com>:
> > This option lets the user provide a file containing information to
> > remove some edges from the trans-opt dependency graph, i.e. the graph
> > used to determine which .trans_opt files to read when making a module's
> > own .trans_opt file.
> > 
> > The reason to remove edges from the graph is to break dependency cycles.
> > .trans_opt files for modules within an SCC have to be made one after
> > another, instead of in parallel. For example, the standard library
> > contains one large SCC due to circular imports, so making .trans_opt
> > files turns out to be a bottleneck when building the standard library
> > (on a machine with sufficient parallelism available).
> > 
> > Furthermore, the user had no control over which modules in an SCC
> > could read the .trans_opt files of other modules in the same SCC.
> > If a and b happened to import each other, the compiler would always
> > break the cycle by allowing a to read b.trans_opt, but not allow b to
> > read a.trans_opt, simply based on module names. The new option lets the
> > user break the cycle in a way that may improve analysis results.
> > 
> > compiler/options.m:
> >     Add the --trans-opt-deps-spec option.
> > 
> > compiler/generate_dep_d_files.m:
> >     If the option --trans-opt-deps-spec FILE is used, use the
> >     information given in the file to remove some edges from the
> >     trans-opt dependency graph.
> > 
> >     If --generate-module-order is passed, also output the module order
> >     computed from the trans-opt dependency graph to a file. This can be
> >     use to guide the writing of the spec file.
> 
> I would reword the last sentence, to something like this:
> 
> Users may find this a useful starting point when writing their own spec file.
> 

Done.

> > +    % The --trans-opt-deps-spec file shall contain a series of terms
> > +    % of either form:
> > +    %
> > +    %   module_allow_deps(M, [ ALLOW ]).
> > +    %   module_disallow_deps(M, [ DISALLOW ]).
> > +    %
> > +    % where M is a Mercury module name,
> > +    % and ALLOW and DISALLOW are comma-separated lists of module names.
> > +    %
> > +    % To make the file less verbose, `builtin' and `private_builtin' are
> > +    % implicitly included in an ALLOW list unless M is itself `builtin'
> > +    % or `private_builtin'.
> 
> I don't think we discussed this earlier, but I think it is worth considering
> whether this policy should be extended either 
> 
> - to all the library modules whose names contain "builtin". or
> - to all the library modules that the compiler can consider a module to depend on
>   implicitly, which includes all the "x_builtin" modules, but also the others that
>   can be returned by compute_implicit_avail_needs in get_dependencies.m.
> 

Hmm, perhaps.

> > +    % TODO: report errors using error specs
> > +    % TODO: report multiple errors
> 
> If you want, I can do these after you commit.
> 

That will be helpful, thanks.

> > +:- pred parse_trans_opt_deps_spec_term(term::in, maybe_error::out,
> > +    trans_opt_deps_spec::in, trans_opt_deps_spec::out) is det.
> > +
> > +parse_trans_opt_deps_spec_term(Term, Result, !SpecFile) :-
> > +    ( if
> > +        Term = functor(atom(AtomName), [LeftTerm, RightTerm], _Context),
> > +        (
> > +            AtomName = "module_allow_deps"
> > +        ;
> > +            AtomName = "module_disallow_deps"
> > +        ),
> > +        try_parse_symbol_name(LeftTerm, SourceName)
> > +    then
> > +        ( if
> > +            AtomName = "module_allow_deps",
> > +            SourceName \= unqualified("builtin"),
> > +            SourceName \= unqualified("private_builtin")
> > +        then
> > +            TargetList0 = [
> > +                unqualified("builtin"),
> > +                unqualified("private_builtin")
> > +            ]
> > +        else
> > +            TargetList0 = []
> > +        ),
> > +        parse_trans_opt_deps_spec_module_list(RightTerm, Result0,
> > +            TargetList0, TargetList),
> > +        (
> > +            Result0 = ok,
> > +            set.list_to_set(TargetList, TargetSet),
> > +            (
> > +                AtomName = "module_allow_deps",
> > +                AllowOrDisallow = module_allow_deps(TargetSet)
> > +            ;
> > +                AtomName = "module_disallow_deps",
> > +                AllowOrDisallow = module_disallow_deps(TargetSet)
> > +            ),
> 
> This code seems to include builtin and private_builtin automatically
> in DISALLOW lists, as well as ALLOW lists.

TargetList0 = [] unless AtomName = "module_allow_deps".

But I've simplified the code anyway.

> > diff --git a/compiler/mercury_compile_make_hlds.m b/compiler/mercury_compile_make_hlds.m
> > index 5b61cbf7e..afe5af779 100644
> > --- a/compiler/mercury_compile_make_hlds.m
> > +++ b/compiler/mercury_compile_make_hlds.m
> 
> > @@ -489,14 +498,14 @@ maybe_grab_plain_and_trans_opt_files(ProgressStream, ErrorStream, Globals,
> >      (
> >          OpModeAugment = opmau_make_trans_opt,
> >          (
> > -            MaybeTransOptDeps = yes(TransOptDeps),
> > +            MaybeDFileTransOptDeps = yes(DFileTransOptDeps),
> >              % When creating the trans_opt file, only import the
> > -            % trans_opt files which are lower in the ordering.
> > +            % trans_opt files which are listed in the `.d' file.
> 
> "listed in the .d file" means "anywhere in the .d file". Is this what you
> want to say?
> 

I have expanded that to:

    % When creating the trans_opt file, only import the
    % trans_opt files which are listed as dependencies of the
    % trans_opt_deps rule in the `.d' file.

> > +            TransOptRuleInfo = trans_opt_deps_from_d_file(DFileTransOptDeps),
> > +            TransOptDeps = DFileTransOptDeps
> > +        ),
> >          % Note that maybe_read_dependency_file searches for
> >          % this exact pattern.
> >          make_module_file_names_with_suffix(Globals,
> >              ext_other(other_ext(".trans_opt")),
> > -            set.to_sorted_list(TransOptDateDeps), TransOptDateDepsFileNames,
> > +            set.to_sorted_list(TransOptDeps), TransOptDepsFileNames,
> >              !IO),
> 
> This change seems strange, but it seems that the original variable name
> was simply badly chosen.
> 

Perhaps it was intended as "the deps of the trans_opt_date rule"?

I've followed your other suggestions, thanks.

I've attached the spec file I'm working on, in case it will be helpful to
you.

Peter
-------------- next part --------------
module_allow_deps(private_builtin, []).
module_allow_deps(builtin, [private_builtin]).

%--------------------------------------%

module_disallow_deps(univ, [
    list,
    require,
    string,
    type_desc
    ]).

module_disallow_deps(exception, [
    io,
    list,
    solutions,
    stm_builtin,
    store,
    string
    ]).

module_disallow_deps(require, [
    enum,
    list,
    string,
    string.format,
    string.parse_util,
    type_desc
    ]).

%--------------------------------------%

module_disallow_deps(int, [
    array,              % only for modes
    pretty_printer
    ]).
module_disallow_deps(int16, [pretty_printer]).
module_disallow_deps(int32, [pretty_printer]).
module_disallow_deps(int64, [pretty_printer]).
module_disallow_deps(int8, [pretty_printer]).

module_disallow_deps(uint, [pretty_printer]).
module_disallow_deps(uint16, [pretty_printer]).
module_disallow_deps(uint32, [pretty_printer]).
module_disallow_deps(uint64, [pretty_printer]).
module_disallow_deps(uint8, [pretty_printer]).

module_disallow_deps(array, [
    pretty_printer,
    string.format,
    string.parse_runtime,
    string.parse_util,
    type_desc           % only for dynamic_cast_to_array
    ]).

module_disallow_deps(char, [pretty_printer]).
module_disallow_deps(float, [pretty_printer]).
module_disallow_deps(one_or_more, [pretty_printer]).
module_disallow_deps(version_array, [pretty_printer]).

%--------------------------------------%

module_disallow_deps(rtti_implementation, [
    bitmap,
    deconstruct,
    string.format,
    string.parse_runtime,
    string.parse_util,
    term_io,            % only for term_io.quoted_string
    type_desc
    ]).

module_disallow_deps(type_desc, []).

%--------------------------------------%

module_disallow_deps(maybe, [list]).
module_disallow_deps(list, [
    pretty_printer,
    set_tree234,
    string,
    term
    ]).

%--------------------------------------%

module_disallow_deps(string, [
    assoc_list,
    deconstruct,
    pretty_printer,
    string.format,
    string.parse_util,
    string.to_string
    ]).
module_disallow_deps(string.parse_util, [
    deconstruct,
    pretty_printer,
    string.format,
    string.to_string
    ]).
module_disallow_deps(string.parse_runtime, [
    deconstruct,
    pretty_printer,
    string.format,
    string.to_string
    ]).
module_disallow_deps(string.format, [
    deconstruct,
    pretty_printer,
    string.to_string
    ]).
module_disallow_deps(string.to_string, []).

%--------------------------------------%

module_disallow_deps(tree234, [
    io,                 % only for trace goals
    pretty_printer,
    term                % only for var type
    ]).

module_disallow_deps(map, [term]).
module_disallow_deps(set, [term]).
module_disallow_deps(set_ordlist, [term]).
module_disallow_deps(set_tree234, [term]).

module_disallow_deps(term, [
    term_int,
    term_subst,
    term_unify,
    term_vars
    ]).

module_disallow_deps(term_conversion, [bitmap]).

%--------------------------------------%

% These only import io for the io.state.
module_disallow_deps(table_builtin, [io]).
module_disallow_deps(time, [io]).

% io.file, io.environment and io.call_system have nothing to do with
% stream I/O.
module_disallow_deps(io.file, [
    benchmarking,
    bitmap,
    dir,
    mercury_term_parser,
    stream.string_writer,
    io,                 % XXX move is_error, etc.
    io.call_system,
    io.environment,
    io.primitives_read,
    io.primitives_write,
    io.stream_db,
    io.stream_ops,
    io.text_read
    ]).
module_disallow_deps(io.environment, [
    benchmarking,
    bitmap,
    dir,
    mercury_term_parser,
    stream.string_writer,
    io,
    io.call_system,
    io.file,
    io.primitives_read,
    io.primitives_write,
    io.stream_db,
    io.stream_ops,
    io.text_read
    ]).
module_disallow_deps(io.call_system, [
    benchmarking,
    bitmap,
    dir,
    mercury_term_parser,
    stream.string_writer,
    io,
    io.environment,
    io.file,
    io.primitives_read,
    io.primitives_write,
    io.stream_db,
    io.stream_ops,
    io.text_read
    ]).

module_disallow_deps(io.stream_db, [
    benchmarking,
    bitmap,
    dir,
    mercury_term_parser,
    stream.string_writer,
    io,
    io.primitives_read,
    io.primitives_write,
    io.stream_ops,
    io.text_read
    ]).
module_disallow_deps(io.stream_ops, [
    benchmarking,
    bitmap,
    dir,
    mercury_term_parser,
    stream.string_writer,
    io,
    io.stream_db,
    io.primitives_read,
    io.primitives_write,
    io.text_read
    ]).
module_disallow_deps(io.primitives_read, [
    benchmarking,
    bitmap,
    dir,
    mercury_term_parser,
    stream.string_writer,
    io,
    io.primitives_write,
    io.text_read
    ]).
module_disallow_deps(io.primitives_write, [
    benchmarking,
    bitmap,
    dir,
    mercury_term_parser,
    stream.string_writer,
    io,
    io.primitives_read,
    io.text_read
    ]).
module_disallow_deps(io, [
    benchmarking,
    bitmap,
    dir,
    mercury_term_parser,
    stream.string_writer,
    io.text_read,
    type_desc       % only for gc_init
    ]).
module_disallow_deps(io.text_read, []).

% term_io calls stream.string_writer.maybe_write_paren.
% stream.string_writer calls term_io.quote_X.
module_disallow_deps(term_io, [stream.string_writer]).
module_disallow_deps(stream.string_writer, []).

%--------------------------------------%