[m-rev.] for preliminary review: Add option to break trans-opt dependencies.

[Peter Wang]

On Mon, 19 Dec 2022 22:40:50 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> 2022-12-19 17:48 GMT+11:00 "Peter Wang" <novalazy at gmail.com>:
> > Here is a patch to improve parallel making of .trans_opt files.
> > The naming of things is rather clunky.
> 
> My prelim review is attached.
> 
> > Here are times for "mmake -j32 trans_opts" in the library directory,
> > using the attached mer_std.make-trans-opt-deps file:
> > 
> > - without the new option
> > 
> >     real        0m39.742s
> >     user        2m11.086s
> >     sys         0m5.166s
> > 
> > - with --make-trans-opt-deps-spec
> > 
> >     real        0m18.700s
> >     user        1m42.337s
> >     sys         0m4.331s
> 
> That is nice. What do you think is the cause of the
> reductions in user and sys times? Is it just fewer
> .trans_opt files being read, or is there some other factor as well?

That is my guess as well.

> > The information of interest in the mer_std.make-trans-opt-deps file is
> > all in the %-commented out lines.
> 
> What is the purpose of the /* */ commented out lines?
> 

There wasn't any need to remove outgoing edges from those modules
to end up with an acyclic graph.

> For the bitmap regression and possibly others, we could fix it
> by moving is_error, throw_on_error and their variants, along with
> the types specific to their operations, to a new module. We could
> keep forwarding predicates, marked obsolete, in io.m for now,
> but redirect all references from the Mercury stdlib now.
> 

Good idea. Forwarding predicates are not required, though.
is_error, throw_on_error, etc. are not part of the public interface.

> > For a given source module, we want to
> > prevent the compiler reading another module's .trans_opt file.
> > Therefore, I'm thinking the file should state the negative dependencies
> > directly, e.g.
> > 
> >     foo - {
> >         not bar,
> >         not baz
> >     }
> 
> I am not a fan of using generic constructors, such as - and {},
> to encode specific semantics; that's why I have been replacing bools
> in the compiler with purpose-specific types for a while now.
> I would prefer using terms such as
> 
>    module_allowed_deps(foo, [x, y])
>    module_nonallowed_deps(foo, [bar, baz])
> 
> In fact, we could allow the deps spec file to contain any entry
> for a module using either (but obviously not both) of these two forms.
> 

Ok.

> I would also change the overall deps file syntax from
> 
>    { entry, entry, ... }
> 
> where each entry is one of the above, to just
> 
>    entry.
>    entry.
>    ...
> 
> This would eliminate the outer {}s which have no real purpose,
> and allow all entries to be written out without having to keep track
> of whether the entry being written out is the last one (since the last gets
> a close brace put after them, not a comma).
> 
> This would also get rid of the "something after the one term we expect"
> error type, replacing it with the already existing "malformed entry" error type.

Ok.

> > This is of course similar to my pragma no_trans_opt proposal,
> > only contained in a single file (arguably better).
> 
> I think it is better. And supporting the specification of both allowed
> and nonallowed deps satisfies both your preference and mine.

We would want to state the deps of builtin and private_builtin
as "allow" lists anyway, which I hadn't considered.

> > +:- type make_trans_opt_deps_spec == map(module_name, set(module_name)).
> > +
> > +    % The --make-trans-opts-deps-spec file shall contain a term of the form:
> 
> Why is "make" part of these names? Shouldn't the mmc --make and mmake
> treat .trans_opt files the same?

The "make-trans-opt" part referred to the "--make-trans-opt" option.
The file affects the dependencies when making a .trans_opt file, but not
which .trans_opt files will be read when generating target code.

I will change the option to --trans-opt-deps-spec. Other suggestions are
welcome.

> > +    %   { PAIRS... }.
> > +    %
> > +    % where PAIRS is a comma-separated list of pairs:
> > +    %
> > +    %   M - { ALLOWED... }
> > +    %
> > +    % M is a Mercury module name.
> > +    % ALLOWED is a comma-separated list of module names.
> 
> Why are the ...s doing in the above? They make it look like 
> what is inside each brace is a list of LISTS OF pairs/module names.
> 

Deleted.

> > +    % Such a pair specifies that for the module M,
> > +    % if it has a cyclic dependency on T,
> 
> I think this would be simpler described as "if module M and T depend on
> each other, directly or indirectly".
> 
> > +    % then the edge M -> T is removed from the dependency graph
> > +    % for the purposes of making trans-opt files.
> 
> Delete this, ...
> 
> > +    ... in the process of making M.trans_opt,
> > +    % the compiler may not read T.trans_opt.
> 
> and keep just this.
> 

I followed your suggestions.

> > +    % To make the file less verbose, `builtin' and `private_builtin' are
> > +    % implicitly included in the ALLOWED list unless M is itself
> > +    % `builtin' or `private_builtin'.
> > +    %
> > +:- pred read_make_trans_opt_deps_spec(string::in,
> > +    maybe_error(make_trans_opt_deps_spec)::out, io::di, io::uo) is det.
> 
> As above, this name would be better without the "make" part.
> Likewise for the predicates below.

Done.

> 
> > +read_make_trans_opt_deps_spec(FileName, Result, !IO) :-
> > +    io.read_named_file_as_string(FileName, ReadResult, !IO),
> > +    (
> > +        ReadResult = ok(Contents),
> > +        read_term_from_string(FileName, Contents, _EndPos, ReadTerm),
> > +        (
> > +            ReadTerm = eof,
> > +            Result = error("expected term, got EOF")
> > +        ;
> > +            ReadTerm = error(Error, LineNum),
> > +            string.format("line %d: %s", [i(LineNum), s(Error)], Msg),
> > +            Result = error(Msg)
> > +        ;
> > +            ReadTerm = term(_VarSet, Term),
> > +            parse_make_trans_opt_deps_spec(Term, ParseResult, map.init, Spec),
> > +            (
> > +                ParseResult = ok,
> > +                % XXX check for EOF
> > +                Result = ok(Spec)
> 
> I don't see any point in deferring acting on this XXX; it needs only
> a few lines of code.
> 

TODO

> > +parse_make_trans_opt_deps_spec(Term, Result, !Spec) :-
> > +    ( if Term = functor(atom("{}"), TermArgs, _Context) then
> > +        parse_make_trans_opt_deps_spec_pairs(TermArgs, Result, !Spec)
> > +    else
> > +        get_term_context(Term) = context(_FileName, LineNum),
> > +        string.format("line %d: expected {}", [i(LineNum)], Msg),
> > +        Result = error(Msg)
> > +    ).
> 
> I would include the file name in the error message, in our usual format,
> i.e. "FileName:LineNum: Msg".

TODO

> However, it would be even better if you
> constructed an error_spec. Same for every place below where parsing fails.
> 

I will defer this to a later change, once the functionality and file
format are finalised.

> To make this less confusing, you would have to rename the variables
> representing make_trans_opt_deps_specs to something other than "Spec".
> And maybe the type as well. Some name involving "allowed edges/deps",
> perhaps?
> 

> > +:- pred parse_make_trans_opt_deps_spec_pairs(list(term)::in, maybe_error::out,
> > +    make_trans_opt_deps_spec::in, make_trans_opt_deps_spec::out) is det.
> > +
> > +parse_make_trans_opt_deps_spec_pairs([], ok, !Spec).
> > +parse_make_trans_opt_deps_spec_pairs([Term | Terms], Result, !Spec) :-
> > +    parse_make_trans_opt_deps_spec_pair(Term, Result0, !Spec),
> > +    (
> > +        Result0 = ok,
> > +        parse_make_trans_opt_deps_spec_pairs(Terms, Result, !Spec)
> > +    ;
> > +        Result0 = error(Error),
> > +        Result = error(Error)
> > +    ).
> 
> If you can't parse a pair, it would be better to record an error_spec and
> keep going, to find and diagnose any later problems in one compiler invocation.
> 

I will defer this to a later change.

> > +:- pred remove_edges_for_make_trans_opt_deps(make_trans_opt_deps_spec::in,
> > +    digraph(module_name)::in, digraph(module_name)::out) is det.
> > +
> > +remove_edges_for_make_trans_opt_deps(Spec, !Graph) :-
> > +    SCCs = set.to_sorted_list(digraph.cliques(!.Graph)),
> > +    map.foldl(remove_edges_for_make_trans_opt_deps_2(SCCs), Spec, !Graph).
> 
> Wouldn't it be easier to
> 
> - loop over SCCs, and
> - in each SCC, loop over the modules in that SCC, looking them up in Spec?

Done.

> 
> After applying a key-value pair in Spec, you could delete it from Spec,
> which would allow you to find and report any entries in the file that
> talk about nonexistent modules.
> 

TODO

> > diff --git a/compiler/options.m b/compiler/options.m
> > index 7d27b7819..def491228 100644
> > --- a/compiler/options.m
> > +++ b/compiler/options.m
> > @@ -387,6 +387,7 @@
> >      ;       show_developer_type_repns
> >      ;       show_dependency_graph
> >      ;       imports_graph
> > +    ;       make_trans_opt_deps_spec
> >      ;       dump_trace_counts
> >      ;       dump_hlds
> >      ;       dump_hlds_pred_id
> > @@ -1436,6 +1437,7 @@ optdef(oc_aux_output, show_all_type_repns,              bool(no)).
> >  optdef(oc_aux_output, show_developer_type_repns,        bool(no)).
> >  optdef(oc_aux_output, show_dependency_graph,            bool(no)).
> >  optdef(oc_aux_output, imports_graph,                    bool(no)).
> > +optdef(oc_aux_output, make_trans_opt_deps_spec,         maybe_string(no)).
> >  optdef(oc_aux_output, dump_trace_counts,                accumulating([])).
> >  optdef(oc_aux_output, dump_hlds,                        accumulating([])).
> >  optdef(oc_aux_output, dump_hlds_pred_id,                accumulating([])).
> > @@ -2388,6 +2390,7 @@ long_option("show-developer-type-repns",            show_developer_type_repns).
> >  long_option("show-developer-type-representations",  show_developer_type_repns).
> >  long_option("show-dependency-graph",    show_dependency_graph).
> >  long_option("imports-graph",            imports_graph).
> > +long_option("make-trans-opt-deps-spec", make_trans_opt_deps_spec).
> >  long_option("dump-trace-counts",        dump_trace_counts).
> >  long_option("dump-hlds",                dump_hlds).
> >  long_option("hlds-dump",                dump_hlds).
> 
> Again, I would delete "make" from the option name.
> 

Done.

> > diff --git a/compiler/write_deps_file.m b/compiler/write_deps_file.m
> > index 6cb5cbcda..d74eb4da6 100644
> > --- a/compiler/write_deps_file.m
> > +++ b/compiler/write_deps_file.m
> > @@ -32,10 +32,11 @@
> >  :- type maybe_intermod_deps
> >      --->    no_intermod_deps
> >      ;       intermod_deps(
> > -                id_int_deps         :: set(module_name),
> > -                id_imp_deps         :: set(module_name),
> > -                id_indirect_deps    :: set(module_name),
> > -                id_fim_deps         :: set(module_name)
> > +                id_int_deps             :: set(module_name),
> > +                id_imp_deps             :: set(module_name),
> > +                id_indirect_deps        :: set(module_name),
> > +                id_fim_deps             :: set(module_name),
> > +                id_make_trans_opt_deps  :: set(module_name)
> >              ).
> 
> And here.
> 

"id_make_trans_opt_deps" is only relevant when making a .trans_opt file.
I've added a comment.

I'll resume work on this change next year.

Peter