[m-rev.] for review: minimal model tabling analysis

[Zoltan Somogyi]

On 30-May-2006, Julien Fischer <juliensf at cs.mu.OZ.AU> wrote:
> -    ;       will_not_modify_trail.
> +    ;       will_not_modify_trail
>                              % This goal will not modify the trail, so it
>                              % is safe for the compiler to omit trailing
>                              % primitives when generating code for this goal.
> -
> +
> +    ;       cannot_call_mm_tabled.
> +                            % This goal will never call a procedure that
> +                            % is evaluted using minimal model tabling.  It
> +                            % is safe for the code generator to omit the
> +                            % pneg context wrappers when generating code for
> +                            % this goal.

Is there some reason why the new feature isn't called will_not_call_mm_tabled?
That would be more consistent with will_not_modify_trail.

> +    % Map from a proc to a indication of whether or not it (or one of
> +    % its subgoals) calls a procedure that is tabled using minimal
> +    % model tabling.
> +    %
> +:- type mm_tabling_info == map(pred_proc_id, proc_mm_tabling_info).
> +
> +:- type proc_mm_tabling_info
> +    --->    proc_mm_tabling_info(
> +                proc_mm_tabling_status :: mm_tabling_status,
> +                proc_maybe_mm_tabling_analysis_status :: maybe(analysis_status)
> +            ).

That field name is too long for my taste. Also, please document what the second
field is there for.


> +check_goal_for_mm_tabling_calls_2(SCC, VarTypes, Goal, _,
> +        Result, MaybeAnalysisStatus, !ModuleInfo, !IO) :-
> +    Goal = call(CalleePredId, CalleeProcId, CallArgs, _, _, _),

I would move the code for this case to its own predicate, and maybe do likewise
for the generic_call and (later) unify cases. All the other cases are just
combining other results.

Same thing in annotate_goal_2 below. Put the corresponding predicates next
to each other, with a comment that updating check_call_for_mm_tabling_calls
will usually require updating annotate_call too, and similarly for the
predicates handing generic_call and unify.

> +    CalleePPId = proc(CalleePredId, CalleeProcId),
> +    module_info_pred_info(!.ModuleInfo, CalleePredId, CalleePredInfo),
> +    (
> +        % Handle (mutually-)recursive calls.
> +        list.member(CalleePPId, SCC)
> +    ->
> +        % XXX user-defined uc - need to handle polymorphic recursion here.
> +        Result = mm_tabled_will_not_call,
> +        MaybeAnalysisStatus = yes(optimal)
> +    ;
> +        pred_info_is_builtin(CalleePredInfo)
> +    ->
> +        Result = mm_tabled_will_not_call,
> +        MaybeAnalysisStatus = yes(optimal)
> +    ;
> +        % Handle builtin unify and compare.
> +        %
> +        % NOTE: the type specific unify and compare predicates are just
> +        %       treated as though they were normal predicates.
> +        %
> +        ModuleName = pred_info_module(CalleePredInfo),
> +        any_mercury_builtin_module(ModuleName),
> +        Name = pred_info_name(CalleePredInfo),
> +        Arity = pred_info_orig_arity(CalleePredInfo),
> +        ( SpecialPredId = spec_pred_compare
> +        ; SpecialPredId = spec_pred_unify
> +        ),
> +        special_pred_name_arity(SpecialPredId, Name, _, Arity)
> +    ->

I would use

	special_pred_name_arity(SpecialPredId, GenericPredName,
		_TypeSpecificPredName, Arity),
	Name = GenericPredName

> +check_goal_for_mm_tabling_calls_2(SCC, VarTypes, Goal, _,
> +        Result, MaybeAnalysisStatus, !ModuleInfo, !IO) :-
> +    Goal = if_then_else(_, If, Then, Else),
> +    check_goals_for_mm_tabling_calls(SCC, VarTypes, [If, Then, Else],
> +        Result0, MaybeAnalysisStatus, !ModuleInfo, !IO),
> +    (
> +        Result0 = mm_tabled_will_not_call,
> +        Result  = mm_tabled_will_not_call
> +    ;
> +        ( Result0 = mm_tabled_conditional
> +        ; Result0 = mm_tabled_may_call
> +        ),
> +        Result = mm_tabled_may_call
> +    ).

Why turn Result0 = mm_tabled_conditional into Result = mm_tabled_may_call?

> +:- pred combine_mm_tabling_status(mm_tabling_status::in,
> +    mm_tabling_status::in, mm_tabling_status::out) is det.
> +
> +combine_mm_tabling_status(mm_tabled_will_not_call, Y, Y).

Please use a more meaningful variable name.

> +:- pred make_opt_int(module_info::in, io::di, io::uo) is det.
> +
> +make_opt_int(ModuleInfo, !IO) :-
> +    module_info_get_name(ModuleInfo, ModuleName),
> +    module_name_to_file_name(ModuleName, ".opt.tmp", no, OptFileName, !IO),
> +    globals.io_lookup_bool_option(verbose, Verbose, !IO),
> +    maybe_write_string(Verbose, "% Appending mm_tabling_info pragmas to `",
> +        !IO),

Why is this a separate pass?

> + at item @samp{will_not_call_mm_tabled/may_call_mm_tabled}
> +This attribute declares whether or not a foreign procedure makes calls
> +back to Mercury procedures that are evaluated using minimal model tabling
> +(see @ref{Tabled evaluation}).  Specifying that a foreign procedure
> +will not call procedures evaluated using minimal model tabling may allow
> +the compiler to generate more efficient code.  In compilation grades that
> +do not support minimal model tabling this attribute is ignored.  These
> +attributes may not be used with procedures that do not make calls back
> +to Mercury, i.e. that have the @samp{will_not_call_mercury} attribute.
> +The default, in case none is specified, is @samp{may_call_mm_tabled}.

This implies that the default is in fact not allowed. Please reword to
eliminate this impression.

Otherwise, the diff is fine. Thanks Julien.

Zoltan.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------