[m-dev.] for review: polymorphic ground insts [1/3]

[Fergus Henderson]

On 09-Feb-2000, David Overton <dmo at ender.cs.mu.oz.au> wrote:
hlds_pred.m:
>  :- type proc_info
>  	--->	procedure(
> -			maybe(determinism),
> +			declared_determinism :: maybe(determinism),
>  					% _declared_ determinism
>  					% or `no' if there was no detism decl
[... 20 lines later ...]
> -			stack_slots,	% stack allocations
> -			determinism,	% _inferred_ determinism
> -			bool,		% no if we must not process this
> +			stack_slots :: stack_slots,	% stack allocations
> +			inferred_determinism :: determinism,
> +					% _inferred_ determinism
> +			can_process :: bool,
> +					% no if we must not process this

It would be a good idea to move the declaration
of the `declared_determinism' field next to the declaration
of the `inferred_determinism' field.  You might want to consider
rearranging the order of the other fields too.
Some of these fields occur in "historical order", because
previously it was very tedious to reorder fields.
Now that we're using records, we should make sure to declare
the fields in the order that makes things easiest to read.

> +proc_info_get_rl_exprn_id(ProcInfo, ProcInfo^rl_exprn_id).
> +
>  % :- type proc_info
>  % 	--->	procedure(
>  % A			maybe(determinism),

You should probably delete that comment now.
(It just duplicates the type declaration,
annotating each field with a letter for use
in the definitions of access predicates.)

> +++ compiler/inst.m	2000/02/04 00:04:23
> @@ -1,5 +1,5 @@
>  %-----------------------------------------------------------------------------%
> -% Copyright (C) 1997, 1999 The University of Melbourne.
> +% Copyright (C) 1997, 2000 The University of Melbourne.

That should be

% Copyright (C) 1997, 1999-2000 The University of Melbourne.

> @@ -34,7 +34,7 @@
>  	;		free(type)
>  	;		bound(uniqueness, list(bound_inst))
>  				% The list(bound_inst) must be sorted
> -	;		ground(uniqueness, maybe(pred_inst_info))
> +	;		ground(uniqueness, ground_inst_info)
>  				% The pred_inst_info is used for
>  				% higher-order pred modes

That comment is unclear now.  You should update it to reflect
your change.

> +:- type ground_inst_info
> +	--->	higher_order(pred_inst_info)
> +	;	constrained_inst_var(inst_var)
> +	;	none.

Some comments here would also be helpful.

> +++ compiler/inst_match.m	2000/02/08 04:57:39
> @@ -1,5 +1,5 @@
>  %-----------------------------------------------------------------------------%
> -% Copyright (C) 1995-1998 The University of Melbourne.
> +% Copyright (C) 1995-2000 The University of Melbourne.

That should probably be "1995-1998, 2000".

> +inst_matches_initial_3(bound(UniqA, ListA),
> +		ground(UniqB, constrained_inst_var(V)), _, _,
> +		ModuleInfo0, ModuleInfo, Sub0, Sub) :-
> +	unique_matches_initial(UniqA, UniqB),
> +	bound_inst_list_is_ground(ListA, ModuleInfo0),
> +	bound_inst_list_matches_uniq(ListA, UniqB, ModuleInfo0),
> +	abstractly_unify_inst(live, bound(UniqA, ListA), ground(UniqB, none),
> +		fake_unify, ModuleInfo0, Inst, _Det, ModuleInfo1),
> +	update_inst_var_sub(V, Inst, ModuleInfo1, ModuleInfo, Sub0, Sub).

Hmm... it might be helpful to have a comment explaining
what the call to update_inst_var_sub does... either that,
or the definition of update_inst_var_sub should have some
detailed comments.

> +inst_matches_initial_3(ground(UniqA, GII), bound(UniqB, List), MaybeType,
> +		Expansions, ModuleInfo0, ModuleInfo, Sub0, Sub) :-

I found it easiest if each variable from the insts was suffixed with
`A' (for the LHS) or `B' (for the RHS).  So I suggest s/GII/GII_A/
and s/List/ListA/g.  Similarly in a few other spots, e.g. you use
`V' in a number of places where I think `InstVarA' or `InstVarB'
would be clearer.

> +:- pred ground_matches_initial_bound_inst_list(uniqueness, list(bound_inst),
> +	maybe(type), expansions, module_info, module_info,
> +	inst_var_sub, inst_var_sub).
> +:- mode ground_matches_initial_bound_inst_list(in, in, in, in, in, out,
> +	in, out) is semidet.

I think you should add a comment here saying that this predicate assumes that
the check of `bound_inst_list_is_complete_for_type' is done by the caller.
Otherwise, the definition of `matches_initial' would lead the reader to
expect that this predicate should perform that check.

> +:- pred bound_inst_list_is_complete_for_type(set(inst_name), module_info,
> +		list(bound_inst), type).
> +:- mode bound_inst_list_is_complete_for_type(in, in, in, in) is semidet.

It would be helpful to have a comment here explaining what it means for a
inst or a bound_inst_list to be "complete" for a type.

> +inst_is_complete_for_type(Expansions, ModuleInfo, Inst, Type) :-
> +	( Inst = defined_inst(InstName) ->
> +		( set__member(InstName, Expansions) ->
> +			true
> +		;
> +			inst_lookup(ModuleInfo, InstName, ExpandedInst),
> +			inst_is_complete_for_type(Expansions `insert` InstName,
> +				ModuleInfo, ExpandedInst, Type)

I think it would be best to keep the module qualifier, i.e.
make that "`set__insert`" rather than just "`insert`".

> +		)
> +	; Inst = bound(_, List) ->
> +		bound_inst_list_is_complete_for_type(Expansions, ModuleInfo,
> +			List, Type)
> +	;
> +		true
> +	).

I think that logically speaking, if Inst = not_reached, then
this predicate should fail.  After all, not_reached is basically
equivalent to a special representation of `bound(_, [])', and
this predicate fails for `bound(_, [])'.

(I think this doesn't affect the behaviour of your changes, since you
check for not_reached elsewhere anyway.  But still...)

> +:- pred equivalent_cons_ids(cons_id, cons_id).
> +:- mode equivalent_cons_ids(in, in) is semidet.

It might be nice to have a comment here explaining what kind of equivalence
this computes.

> +%-----------------------------------------------------------------------------%
> +
> +:- pred update_inst_var_sub(inst_var, inst, module_info, module_info,
> +		inst_var_sub, inst_var_sub).
> +:- mode update_inst_var_sub(in, in, in, out, in, out) is semidet.
> +
> +update_inst_var_sub(V, InstA, ModuleInfo0, ModuleInfo, Sub0, Sub) :-
> +	( map__search(Sub0, V, InstB) ->
> +		inst_merge(InstA, InstB, ModuleInfo0, Inst, ModuleInfo),
> +		map__det_update(Sub0, V, Inst, Sub)
> +	;
> +		ModuleInfo = ModuleInfo0,
> +		map__det_insert(Sub0, V, InstA, Sub)
> +	).

It would be a good idea to document this predicate better.
For example, what is it's purpose?  And why does it use `inst_merge'?

> +%-----------------------------------------------------------------------------%
> +
> +:- pred ground_inst_info_matches_initial(ground_inst_info, ground_inst_info,
> +		uniqueness, uniqueness, maybe(type), expansions,
> +		module_info, module_info, inst_var_sub, inst_var_sub).
> +:- mode ground_inst_info_matches_initial(in, in, in, in, in, in, in, out, in,
> +		out) is semidet.
> +
> +ground_inst_info_matches_initial(_, none, _, _, _, _, M, M) --> [].
> +ground_inst_info_matches_initial(higher_order(PredInstA),
> +		higher_order(PredInstB), _, _, Type, Expansions,
> +		ModuleInfo0, ModuleInfo) -->
> +	pred_inst_matches_initial(PredInstA, PredInstB, Type, Expansions,
> +		ModuleInfo0, ModuleInfo).

Why do you ignore the uniquenesses here?
If this predicate is not supposed to check the uniquenesses,
that should be documented.

> +ground_inst_info_matches_initial(GroundInstInfoA, constrained_inst_var(V),
> +		UniqA, UniqB, _, _, ModuleInfo0, ModuleInfo) -->
> +	{ GroundInstInfoA = constrained_inst_var(_) ->
> +		Inst = ground(UniqA, GroundInstInfoA),
> +		ModuleInfo1 = ModuleInfo0
> +	;
> +		abstractly_unify_inst(live, ground(UniqA, GroundInstInfoA),
> +			ground(UniqB, none), fake_unify, ModuleInfo0, Inst,
> +			_Det, ModuleInfo1)
> +	},

Why is the if-then-else there needed?
Hmm, you ignore the uniqueness in the "then" case, but you check
the uniqueness in the "else" case... why?
Is there any other difference between the two cases?

> +:- pred maybe_get_arg_types(module_info, maybe(type), cons_id,
> +		list(maybe(type))).
> +:- mode maybe_get_arg_types(in, in, in, out) is det.
> +
> +maybe_get_arg_types(ModuleInfo, MaybeType, ConsId0, MaybeTypes) :-
> +	( ConsId0 = cons(SymName, Arity) ->
> +		( SymName = qualified(_, Name) ->
> +			% type_util__get_cons_id_arg_types expects an 
> +			% unqualified cons_id.

It would be a good idea to add that comment to type_util.m too.

> @@ -551,9 +804,9 @@
>  		% insts in ListB, or if ListB does not contain a complete list
>  		% of all the constructors for the type in question.
>  	%%% error("not implemented: `ground' matches_final `bound(...)'").

Did you consider fixing this bug in the same way as you fixed
the similar one in inst_matches_initial?

> +ground_inst_info_matches_final(constrained_inst_var(I),
> +		constrained_inst_var(I), _, _).

Hmm...

> +ground_inst_info_matches_binding(constrained_inst_var(_),
> +		constrained_inst_var(_), _). % AAA

Hmm again...  what's the "AAA" comment for?


[... to be continued ...]

-- 
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.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------