[m-dev.] for review: Aditi [2]

[Fergus Henderson]

On 07-Jul-1998, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> +:- type aditi_dependency_ordering       == list(aditi_sub_module).
> +
> +        % Each Aditi SCC contains one or more SCCs from the original 
> +        % dependency ordering and the entry points of the SCC.
> +:- type aditi_sub_module
> +        --->    aditi_sub_module(dependency_ordering, list(pred_proc_id)).

The use of the term "sub_module" is confusing now that Mercury
has nested modules.  I suggest you use "SCC" instead.

You should document what defines an valid Aditi SCC.

>  :- type dependency_info --->
>                  dependency_info(
>                          dependency_graph,       % Dependency graph
> -                        dependency_ordering     % Dependency ordering
> +                        dependency_ordering,    % Dependency ordering
> +                        maybe(aditi_dependency_ordering)
> +                                                % Dependency ordering of Aditi
> +                                                % SCCs with some merged
>                  ).


This documentation sort of begs the question "which ones are merged?".

> --- hlds_pred.m	1998/05/15 07:07:10	1.48
> +++ hlds_pred.m	1998/06/04 06:29:24
> @@ -204,10 +204,47 @@
> +        ;       memo            % Requests that this predicate be evaluated
> +                                % using memoing.
> +        ;       no_memo         % Ensure that this procedure is not memoed.

Hmm, there's already an eval_method of eval_memo in the proc_info;
how do these two relate?

The comment here refers to procedure, but the flag applies to predicates,
not procedures.

> +                        % `naive' and `psn' are mutually exclusive.
> +        ;       naive           % Use naive evaluation.
> +        ;       psn             % Use predicate semi-naive evaluation.

It would be helpful to mention Aditi in the comment(s) here.
Comments here saying "see ... for details" would be helpful.

> +                        % `context' and `supp_magic' are mutually
> +                        % exclusive. One of them must be performed
> +                        % on all Aditi procedures. `supp_magic'
> +                        % is the default
> +
> +        ;       supp_magic      % Perform the supplementary magic sets
> +                                % transformation on this predicate. 
> +        ;       context         % Perform the context transformation on
> +                                % the predicate. 

Comments here saying "see ... for details" would be helpful.

> +        ;       generate_inline % Used for small procedures which project
> +                                % a relation to be used as input to a call.
> +                                % The goal should consist of fail, true
> +                                % or a single rule.
> +                                % These relations are never memoed.
> +                                % The reason these must be generated inline
> +                                % is to ensure that the relation used for
> +                                % input is a projection of the current 
> +                                % value of the projected relation for all
> +                                % orderings of the SCC.

I must confess that the justification given here has me mystified.

> +        ;       aditi_interface % No code is actually generated for this
> +                                % procedure type. A call to a procedure with
> +                                % this marker is generated as a call to
> +                                % do_call_aditi_*, which is defined in hand
> +                                % coded C in extras/aditi/aditi.m.
> +
> +                                % The default flags for Aditi procedures are
> +                                % aditi, dnf, supp_magic, psn and memo.

This comment is easily missed; I suggest you move it to just before all
the Aditi flag definitions and unindent it.

You're talking about procedures instead of predicates again.

>  :- pred pred_info_init(module_name, sym_name, arity, tvarset, list(type),
>          condition, term__context, clauses_info, import_status,
>          pred_markers, goal_type, pred_or_func, list(class_constraint), 
> -        map(class_constraint, constraint_proof), pred_info).
> +        map(class_constraint, constraint_proof), string, pred_info).

You should document the meaning of the string.  For all the others
parameters, the meaning should (hopefully) be evident from the type,
but for `string' it is not clear.
One way of making it clear would be to define a type

	:- type aditi_owner == string.

and then use that.

(Likewise for pred_info_create.)

> +:- pred hlds_pred__pred_info_is_aditi_aggregate(pred_info).
> +:- mode hlds_pred__pred_info_is_aditi_aggregate(in) is semidet.

What's an Aditi aggregate, exactly?

> +:- pred hlds_pred__is_memoed(module_info, pred_id).
> +:- mode hlds_pred__is_memoed(in, in) is semidet.
> +
> +:- pred hlds_pred__is_differential(module_info, pred_id).
> +:- mode hlds_pred__is_differential(in, in) is semidet.

What do "memoed" and "differential" mean here?

> +hlds_pred__is_memoed(ModuleInfo, PredId) :-
> +        module_info_pred_info(ModuleInfo, PredId, PredInfo),
> +        pred_info_get_markers(PredInfo, Markers),
> +        (
> +                check_marker(Markers, memo)
> +        ;
> +                % Memoing is the default for Aditi procedures.
> +                semidet_fail,   % XXXXXXXX
> +                check_marker(Markers, aditi),
> +                \+ check_marker(Markers, no_memo)
> +        ).

The "% XXXXXXXX" should be explained.
(And three X's should be enough ;-)

> Index: compiler/lambda.m
>                  list__append(ArgModes1, Modes, AllArgModes),
>                  map__apply_to_list(AllArgVars, VarTypes, ArgTypes),
>  
> +                ( 
> +                        % Pass through the aditi markers for 
> +                        % aggregate query closures.
> +                        Detism = nondet,
> +                        check_marker(Markers, aditi)
> +                ->
> +                        LambdaMarkers = Markers
> +                ;
> +                        init_markers(LambdaMarkers)
> +                ),

Why only `nondet'?  What about `multi', for example?

> +        % If there are local Aditi procedures enable Aditi compilation.
> +:- pred maybe_enable_aditi_compilation(item_status, term__context,
> +                module_info, module_info, io__state, io__state) is det.
> +:- mode maybe_enable_aditi_compilation(in, in, in, out, di, uo) is det.
> +
> +maybe_enable_aditi_compilation(Status, Context, Module0, Module) -->
> +        { Status = item_status(ItemStatus, _) },
> +        ( { ItemStatus \= imported } ->

What about opt_imported predicates?

> +                globals__io_lookup_bool_option(aditi, Aditi),
> +                ( { Aditi = no } ->
> +                        prog_out__write_context(Context),
> +                        io__write_string("Error: compilation of Aditi procedures\n"),
> +                        prog_out__write_context(Context),
> +                        io__write_string("  requires the --aditi option.\n"),

s/--aditi/`--aditi'/

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>  |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3        |     -- the last words of T. S. Garp.