[m-dev.] for review: prelim support for hash consing

[David Glen JEFFERY]

On 20-Aug-1999, Peter Ross <petdr at cs.mu.OZ.AU> wrote:
> 
> Estimated hours taken: 15
> 
> Add preliminary support for memoing type constructors (hash consing)

You should mention what this support is: you generate a pred to do the
construction unification for each hash_consed constructor. I guess that
the intention is to using tabling to get hash_consing for free. Is that
right? That is what I gather from the diff... and if so it needs to be
explained somewhere.

> compiler/make_hlds.m:
>     The core of the change.
>     For every 
>         :- pragma memo_cons(Constructor/Arity).
>     Construct a function
>         hash_cons_C(X1, ..., XN) = C(X1, ..., XN).
>     where hash_cons_C is a suitably name mangled function.

Perhaps hash_cons should be the name of this pragma? Anyhow, you should
probably add a sentence saying that this hash_cons_C pred is intended to
replace all construction unifications.

> compiler/hlds_pred.m:
>     Add a new marker which indicates that the predicate is an
>     automatically generated predicate which is meant to do a hash consed
>     construction unification.
> 
> compiler/hlds_out.m:
> compiler/intermod.m:
>     Support the memo_cons marker.
> 
> compiler/mercury_to_mercury.m:
>     Handle the pragma memo_cons.
> 
> compiler/module_qual.m:
> compiler/modules.m:
>     Handle the new type tabled_cons.
> 
> compiler/prog_data.m:
>     Add type tabled_cons which stores the information declared in the
>     pragma.
> 
> compiler/prog_io_pragma.m:
>     Parse the memo_cons pragma.
> 
> compiler/prog_out.m:
>     Add a new predicate sym_name_to_term, which given a symname
>     constructs a valid prog_term for that name.
> 
> Index: intermod.m
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/compiler/intermod.m,v
> retrieving revision 1.66
> diff -u -r1.66 intermod.m
> --- intermod.m	1999/07/13 08:53:01	1.66
> +++ intermod.m	1999/08/19 12:53:43
> @@ -1188,6 +1188,8 @@
>  intermod__should_output_marker(generate_inline, _) :-
>  	% This marker should only occur after the magic sets transformation.
>  	error("intermod__should_output_marker: generate_inline").
> +	% XXX is this correct?
> +intermod__should_output_marker(memo_cons, yes).

I have no idea. Simon?




> +				{ module_info_incr_errors(Module0, Module) }, 
> +				prog_out__write_context(Context),
> +				io__write_string(
> +					"Error: export status mismatch.\n"),

This error message stinks.

> +				{ Info = Info0 }
> +			)
> +		;
> +			{ module_info_incr_errors(Module0, Module) }, 
> +			prog_out__write_context(Context),
> +			io__write_string("Error: constructor '"),
> +			hlds_out__write_cons_id(cons(Constructor, Arity)),
> +			io__write_string("' in multiple types.\n"),
> +			{ Info = Info0 }
> +
> +			% XXX should list what those types are if -E

This error message really isn't very helpful either... it just tells you that
the type is visible in multiple types. It doesn't mention why that is a problem
(ie. because of the memo_cons pragma). I guess it mentions the line number of
the pragma, but I still think you could say something like:
	Error: pragma memo_cons declaration for constructor `foo/2' which
	       appears in multiple types.

Your XXX is true too.

> +		% XXX Need to handle existential construction
> +		% unifications correctly.
> +	{ ExistQVars = [] },
> +	{ ClassContext = constraints([], []) },

You definately need to address this XXX. I'll talk to you about this
personally.

> +	% XXX this actually be in the module that handles hashconsing

s/this actually/this should actually/

> +	% when it is made.
> +:- pred make_hashcons_func_name(sym_name::in, arity::in, type_id::in,
> +		string::out) is det.
> +
> +make_hashcons_func_name(ConsName, ConsArity, TypeName - TypeArity, Name) :-
> +	prog_out__sym_name_to_string(ConsName, ConsStr),
> +	prog_out__sym_name_to_string(TypeName, TypeStr),
> +	string__format("cons_%s/%d_%s/%d", [s(TypeStr), i(TypeArity),
> +			s(ConsStr), i(ConsArity)], Name).

The mangled name could maybe be a little more robust... I'm not sure what, 
though.

> +:- pred construct_types_and_modes(sym_name::in, arity::in,
> +		type_id::in, hlds_type_defn::in, module_info::in,
> +		type_and_mode::out, list(type_and_mode)::out) is det.

Could you please add a comment to this saying what it does. (I worked it
out after a while, but it deserves a comment).

> +
> +construct_types_and_modes(ConsName, Arity, TypeId, TypeDefn,
> +		ModuleInfo, TypeAndMode, TypesAndModes) :-
> +	hlds_data__get_type_defn_tparams(TypeDefn, TypeParams),
> +	construct_type(TypeId, TypeParams, Type),
> +
> +	type_util__get_cons_id_arg_types(ModuleInfo, Type,
> +			cons(ConsName, Arity), TypeArgs),
> +
> +	P = (pred(T0::in, T::out) is det :- T = type_only(T0)),
> +	P(Type, TypeAndMode),
> +	list__map(P, TypeArgs, TypesAndModes).

Why don't you add it with the correct modes? You know it is (in, in, ..., out).

> Index: prog_io_pragma.m
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/compiler/prog_io_pragma.m,v
> retrieving revision 1.21
> diff -u -r1.21 prog_io_pragma.m
> --- prog_io_pragma.m	1999/07/13 08:53:24	1.21
> +++ prog_io_pragma.m	1999/08/19 08:40:49
> @@ -320,6 +320,13 @@
>  	parse_tabling_pragma(ModuleName, "minimal_model", eval_minimal, 
>  		PragmaTerms, ErrorTerm, Result).
>  
> +parse_pragma_type(ModuleName, "memo_cons", PragmaTerms, ErrorTerm,
> +		_VarSet, Result) :-
> +	parse_simple_pragma(ModuleName, "memo_cons",
> +		lambda([Name::in, Arity::in, Pragma::out] is det,
> +			Pragma = tabled_cons(Name, Arity)),
> +		PragmaTerms, ErrorTerm, Result).

Just to be picky... you should probably use the new HO syntax here.
(ie. `pred' rather than `lambda').

> Index: prog_out.m
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/compiler/prog_out.m,v
> retrieving revision 1.43
> diff -u -r1.43 prog_out.m
> --- prog_out.m	1999/07/13 08:53:25	1.43
> +++ prog_out.m	1999/08/19 08:28:12
> @@ -68,6 +68,12 @@
>  :- pred prog_out__sym_name_to_string(sym_name, string, string).
>  :- mode prog_out__sym_name_to_string(in, in, out) is det.
>  
> +	% sym_name_to_term(SymName, Args, Term):
> +	%	convert a symbol name with an associated list of terms
> +	%	into a term.
> +:- pred prog_out__sym_name_to_term(sym_name, list(prog_term), prog_term).
> +:- mode prog_out__sym_name_to_term(in, in, out) is det.

Isn't there already something like this somewhere else?



dgj
-- 
David Jeffery (dgj at cs.mu.oz.au) | If your thesis is utterly vacuous
PhD student,                    | Use first-order predicate calculus.
Dept. of Comp. Sci. & Soft. Eng.|     With sufficient formality
The University of Melbourne     |     The sheerist banality
Australia                       | Will be hailed by the critics: "Miraculous!"
                                |     -- Anon.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------