[m-dev.] for review: alias branch improvements [2/2]

[Fergus Henderson]

On 23-Jul-1999, David Overton <dmo at cs.mu.OZ.AU> wrote:
> +++ modecheck_call.m	1999/07/21 07:02:11
...
> -:- pred compare_inst(inst, inst, maybe(inst), inst_table, match, module_info).
> -:- mode compare_inst(in, in, in, in, out, in) is det.
> +:- pred compare_inst(inst, inst, maybe(pair(inst, instmap)), inst_table,
> +	alias_map, alias_map, match, module_info).
> +:- mode compare_inst(in, in, in, in, in, out, out, in) is det.
>  
> -compare_inst(InstA, InstB, MaybeArgInst, InstTable, Result, ModuleInfo) :-
> +compare_inst(InstA, InstB, MaybeArgInst, InstTable, AliasMap0, AliasMap,
> +		Result, ModuleInfo) :-
>  	% inst_matches_initial(A,B) succeeds iff
>  	%	A specifies at least as much information
>  	%	and at least as much binding as B --
>  	%	with the exception that `any' matches_initial `free'
>  	% 	and perhaps vice versa.
> -	instmap__init_reachable(InstMapA),	% YYY
> -	instmap__init_reachable(InstMapB),	% YYY
> -	instmap__init_reachable(MaybeInstMap),	% YYY
> +	instmap__init_reachable(InstMapA),
> +	instmap__init_reachable(InstMapB),
> +		% Empty instmaps are ok here because InstA and InstB are
> +		% initial insts of procedures which will have no alias
> +		% substitutions on them.

That strikes me as a fairly fragile assumption.
Is this comment really true?

Is it an invariant that all initial insts of procedures will have no
alias substitutions on them, or are you just assuming that this predicate
won't be called for procedures whose initial insts have alias substitutions?

Either way, the invariant that this predicate will only be called for
insts which are from a procedure's initial insts is an important part
of the interface of this predicate and it should be documented with the
predicate declaration, not just in the body of the predicate.
You should check all of the callers of this predicate to make sure that
they either ensure that it holds or document the same requirement of
their callers.

>  	(
> +		% (It is ok to thread the same AliasMap through both of the
> +		% calls to inst_matches_initial in this predicate because
> +		% the inst_keys in InstA and InstB will not overlap.  This
> +		% is because the inst_keys in InstB were created by
> +		% inst_table_create_sub.)

Likewise here, if you're relying on some invariant then you should document
that in the predicate's interface documentation and you should check that
all callers respect it, perhaps by propagating copies of that documentation
into the callers (and then recursively checking the caller's callers, etc.).

> Index: prog_data.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/prog_data.m,v
> retrieving revision 1.24.2.19
> diff -u -u -r1.24.2.19 prog_data.m
> --- prog_data.m	1999/07/19 01:45:22	1.24.2.19
> +++ prog_data.m	1999/07/21 05:01:17
> @@ -23,9 +23,7 @@
>  % should be defined here, rather than in hlds*.m.
>  
>  :- import_module (inst).
> -:- import_module hlds_data.
> -	% YYY hlds_data needed for inst_table definition.  We should move the
> -	% definition of inst_table somewhere else.
> +:- import_module inst_table.

Why do prog_*.m need to import inst_table.m?

> +:- type other_inst_id.
> +
> +:- type other_inst_id_sub.
> +
> +:- func init_other_inst_id = other_inst_id.
> +
> +:- func inc_other_inst_id(other_inst_id) = other_inst_id.
> +
> +:- func other_inst_id_apply_sub(other_inst_id_sub, other_inst_id)
> +		= other_inst_id.
> +
> +:- func other_inst_id_create_sub(other_inst_id) = other_inst_id_sub.

These need to be documented.

> +++ unify_proc.m	1999/07/23 01:54:32
...
> -:- type unify_req_map == map(unify_proc_id, proc_id).
> +:- type unify_req_map == assoc_list(unify_proc_id, proc_id).

Hmm...

Wouldn't it be more efficient to use
`map(type_id, assoc_list(unify_proc_id, proc_id))'?

> +:- pred unify_proc__search_mode_num_2(unify_req_map, unify_proc_id,
> +		instmap, module_info, proc_id).
> +:- mode unify_proc__search_mode_num_2(in, in, in, in, out) is semidet.
> +
> +unify_proc__search_mode_num_2([UnifyIdA - ProcId0 | Rest], UnifyIdB,
> +		InstMapB, ModuleInfo, ProcId) :-
> +	UnifyIdA = unify_proc_id(_, LIA0, RIA0, InstTableA),
> +	UnifyIdB = unify_proc_id(_, LIB, RIB, InstTableB),

Don't you need to check that the type_ids are equal?
(Or use my suggestion above.)

> +:- pred normalise_unify_id(instmap, module_info, unify_proc_id, unify_proc_id).
> +:- mode normalise_unify_id(in, in, in, out) is det.

You should document this predicate.

> +normalise_unify_insts_2(InstMap, ModuleInfo, OrigInstTable,
> +		InstL0, InstL, InstR0, InstR, InstTable0, InstTable) :-
> +	(
> +		InstL0 = alias(IKL)
> +	->
> +		( InstR0 = alias(IKR) ->

Please be consistent; either

	( cond ->
		...
	
or

	(
		cond
	->
		...

is OK, but please don't mix the two arbitrarily.

> +:- pred inst_remove_aliases(instmap, module_info, inst_table, inst, inst,
> +		inst_table, inst_table).
> +:- mode inst_remove_aliases(in, in, in, in, out, in, out) is det.
> +
> +
> +inst_remove_aliases(_, _, _, any(U), any(U), IT, IT).
> +inst_remove_aliases(_, _, _, free(A), free(A), IT, IT).
> +inst_remove_aliases(_, _, _, free(A, T), free(A, T), IT, IT).
> +inst_remove_aliases(_, _, _, ground(U, P), ground(U, P), IT, IT).
> +inst_remove_aliases(_, _, _, not_reached, not_reached, IT, IT).
> +inst_remove_aliases(_, _, _, inst_var(V), inst_var(V), IT, IT).
> +inst_remove_aliases(InstMap, ModuleInfo, OrigInstTable, alias(IK), Inst,
> +		InstTable0, InstTable) :-
> +	inst_table_get_inst_key_table(OrigInstTable, IKT),
> +	instmap__inst_key_table_lookup(InstMap, IKT, IK, Inst0),
> +	inst_remove_aliases(InstMap, ModuleInfo, OrigInstTable, Inst0, Inst,
> +		InstTable0, InstTable).
> +inst_remove_aliases(InstMap, ModuleInfo, OrigInstTable,
> +		abstract_inst(Name, Insts0), abstract_inst(Name, Insts),
> +		InstTable0, InstTable) :-
> +	insts_remove_aliases(InstMap, ModuleInfo, OrigInstTable, Insts0, Insts,
> +		InstTable0, InstTable).
> +inst_remove_aliases(InstMap, ModuleInfo, OrigInstTable, bound(U, BoundInsts0),
> +		bound(U, BoundInsts), InstTable0, InstTable) :-
> +	bound_insts_remove_aliases(InstMap, ModuleInfo, OrigInstTable,
> +		BoundInsts0, BoundInsts, InstTable0, InstTable).

It's generally more readable if the parameter that you are switching on is
the first argument.

--------------------

Apart from that, it looks fine.  Could you please send a relative
diff when you've addressed those issues?

Thanks,
	Fergus.

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