[m-rev.] for review: calling Mercury from Aditi

[Fergus Henderson]

On 01-Oct-2003, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> 
> Allow Aditi to call Mercury.  At the moment, this involves Aditi
> loading a shared library containing the user's code.
...
> +++ compiler/mlds_to_gcc.m	23 Sep 2003 00:40:03 -0000
> @@ -1938,6 +1938,21 @@
>  	build_rtti_type_name(RttiName, Size, GCC_Type).
>  build_rtti_type(tc_rtti_id(TCRttiName), Size, GCC_Type) -->
>  	build_rtti_type_tc_name(TCRttiName, Size, GCC_Type).
> +build_rtti_type(aditi_rtti_id(_), _, GCC_Type) -->
> +	% typedef struct {
> +	%	MR_Code		*MR_aditi_proc_addr;
> +	%	MR_String	MR_aditi_proc_name;
> +	%	MR_TypeInfo	MR_aditi_input_type_info;
> +	%	MR_TypeInfo	MR_aditi_output_type_info;
> +	%	MR_Determinism	MR_aditi_proc_detism;
> +	% }

s/}/} MR_Aditi_Proc_Info;/

> +	build_struct_type("MR_Aditi_Proc_Info",
> +		['MR_Code*'		- "MR_aditi_proc_addr",
> +		 'MR_ConstString'	- "MR_aditi_proc_name",

The code doesn't match the comment: MR_String vs MR_ConstString.

Also, `MR_Code*' is for pointers to LLDS entry labels;
it should not be used for pointers to MLDS procedures.
`MR_ProcAddr' is the type which should be used for
procedure addresses.

> Index: compiler/options.m
...
> @@ -3053,6 +3056,21 @@
>  		"\tmemory usage information, not call counts.",
>  ********************/
>  	]),
> +	io__write_string("      Aditi\n"),
> +	write_tabbed_lines([
> +		"--aditi",
> +		"\tEnable Aditi compilation. You need to enable this",
> +		"\toption if you are making use of the Aditi deductive",
> +		"\tdatabase interface.",
> +		"--aditi-calls-mercury",
> +		"\tEnable calling arbitrary Mercury code from predicates",
> +		"\twith `:- pragma aditi' declarations."
> +
> +		% --aditi-calls-mercury is not fully implemented.
> +		%"--aditi-calls-mercury",
> +		%"\tEnable calling ordinary Mercury code from Aditi."
> +	]),

If it is not fully implemented, then it should be listed in the
WORK_IN_PROGRESS file.

> +++ compiler/rl.m	23 Sep 2003 00:40:04 -0000
> @@ -1313,8 +1313,27 @@
>  		RecursiveTypes0, RecursiveTypes, Decls0, Decls, ThisType) :-
>  	(
>  		type_to_ctor_and_args(Type, TypeCtor, Args),
> -		type_constructors(Type, ModuleInfo, Ctors)
> +		type_constructors(Type, ModuleInfo, Ctors0)
>  	->
> +		% Sort the constructors. This is needed so that
> +		% the conversion from Aditi to Mercury can just
> +		% call std_util__get_functor without needing to
> +		% search for the functor.
> +		list__sort(
> +		    (pred(Ctor1::in, Ctor2::in, CompareResult::out) is det :-
> +			Ctor1 = ctor(_, _, QName1, Args1),
> +			Ctor2 = ctor(_, _, QName2, Args2),
> +			unqualify_name(QName1, Name1),
> +			unqualify_name(QName2, Name2),
> +			compare(NameResult, Name1, Name2),
> +			( NameResult = (=) ->
> +				compare(CompareResult, list__length(Args1),
> +					list__length(Args2) `with_type` int)
> +			;
> +				CompareResult = NameResult
> +			)
> +		    ), Ctors0, Ctors),

This looks like the ordering of data structures will be different
for Mercury than for Aditi... are you sure that's what you want?

> Index: compiler/rl_exprn.m
...
> +:- pred rl_exprn__goal_is_complex(module_info::in,
> +		instmap::in, list(hlds_goal)::in) is semidet.
> +
> +rl_exprn__goal_is_complex(ModuleInfo, _InstMap, Goals) :-
> +	list__member(Goal, Goals),
> +	goal_contains_goal(Goal, SubGoal),
> +	SubGoal = SubGoalExpr - SubGoalInfo,
> +	(
> +		goal_info_get_determinism(SubGoalInfo, Detism),
> +		determinism_components(Detism, _, at_most_many)
> +	;
> +		SubGoalExpr = call(PredId, ProcId, _, _, _, _),
> +		module_info_pred_info(ModuleInfo, PredId, PredInfo),
> +		\+ rl_exprn__is_builtin(PredId, ProcId, PredInfo)
> +	;
> +		SubGoalExpr = generic_call(_, _, _, _)
> +	;
> +		SubGoalExpr = foreign_proc(_, _, _, _, _, _, _)
> +	;
> +		SubGoalExpr = par_conj(_)
> +	;
> +		SubGoalExpr = unify(_, _, _, Unification, _),
> +		Unification = construct(_, ConsId, _, _, _, _, _),
> +		( ConsId = pred_const(_, _, _)
> +		; ConsId = base_typeclass_info_const(_, _, _, _)
> +		; ConsId = type_ctor_info_const(_, _, _)
> +		; ConsId = tabling_pointer_const(_, _)
> +		)
> +	).
> +
> +:- func rl_exprn__cons_id_is_complex(cons_id) = bool.
> +
> +rl_exprn__cons_id_is_complex(cons(_, _)) = no.
> +rl_exprn__cons_id_is_complex(int_const(_)) = no.
> +rl_exprn__cons_id_is_complex(string_const(_)) = no.
> +rl_exprn__cons_id_is_complex(float_const(_)) = no.
> +rl_exprn__cons_id_is_complex(pred_const(_, _, _)) = yes.
> +rl_exprn__cons_id_is_complex(type_ctor_info_const(_, _, _)) = yes.
> +rl_exprn__cons_id_is_complex(base_typeclass_info_const(_, _, _, _)) = yes.
> +rl_exprn__cons_id_is_complex(type_info_cell_constructor) = yes.
> +rl_exprn__cons_id_is_complex(typeclass_info_cell_constructor) = yes.
> +rl_exprn__cons_id_is_complex(tabling_pointer_const(_, _)) = yes.
> +rl_exprn__cons_id_is_complex(deep_profiling_proc_static(_)) = yes.
> +rl_exprn__cons_id_is_complex(table_io_decl(_)) = yes.

That function appears to be unused.
Probably it ought to be called from the procedure above.

> +%-----------------------------------------------------------------------------%
> +
> +:- pred rl_exprn__generate_top_down_call(rl_goal_inputs::in,
> +	rl_goal_outputs::in, list(hlds_goal)::in, list(bytecode)::out,
> +	int::out, exprn_mode::out, list(type)::out,
> +	rl_exprn_info::in, rl_exprn_info::out) is det.
>
> +rl_exprn__generate_top_down_call(Inputs, MaybeOutputs,
> +		GoalList, Code, NumParams, Mode, Decls) -->
> +	%
> +	% Produce a procedure containing the join condition.
> +	%

Is this comment intended to describe the whole of what
generate_top_down_call does?  If so, then it should go
above the pred declaration, rather than in the clause body.

More comments explaining in more detail what this procedure
is supposed to do would be helpful. 

In particular, I think it would be helpful to say a bit
about the kinds of input code (HLDS?) and output code
(Aditi bytecode... that will call Mercury code which has
been compiled (possibly via C) to native code).

Also, since the details of the generated code are complicated,
I think it would be worth having a comment showing the general
form that the generated code is supposed to take.

Perhaps also comments with each section of the code
saying which bit of the generated code it is generating.

> +:- pred rl_exprn__build_top_down_procedure(list(prog_var)::in,
> +	list(prog_var)::in, list(hlds_goal)::in, string::out, int::out,
> +	rl_exprn_info::in, rl_exprn_info::out) is det.

A comment or two here would also be a good idea, IMHO.

> Index: tests/valid/Mmakefile
> +# Aditi is not yet implemented for the non-C back-ends
> +# (i.e. grades java* il*).

s/non-C/non-C, non-asm/

Or better, say "Aditi is only implemented for the C and asm back-ends"
(i.e. not for grades java* il*)

Otherwise that looks fine.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------