[m-dev.] for review: fixing cse_detection for existential types

[David Glen JEFFERY]

On 06-Dec-2000, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> compiler/cse_detection.m:
> 	Fix common subexpression elimination so that it works when the
> 	deconstruction it hoists out of a branched control structure
> 	involves a functor with existentially typed arguments.
> 
> compiler/options.m:
> 	Put the default value of --compare-specialization back to 4, now
> 	that the bug that prevented it from working at levels above 1 has
> 	been fixed.

This basically looks pretty good. A couple of comments, though.

> cvs diff: Diffing .
> Index: cse_detection.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/cse_detection.m,v
> retrieving revision 1.66
> diff -u -b -r1.66 cse_detection.m
> --- cse_detection.m	2000/12/05 02:03:44	1.66
> +++ cse_detection.m	2000/12/05 23:23:48

> +pair_subterms([], _Context, _UnifyContext, []).
> +pair_subterms([OldVar - HoistedVar | OldHoistedVars], Context, UnifyContext,
> +		Replacements) :-
> +	pair_subterms(OldHoistedVars, Context, UnifyContext, Replacements1),
> +	( OldVar = HoistedVar ->
>  			Replacements = Replacements1
>  		;
>  			UnifyContext = unify_context(MainCtxt, SubCtxt),
> -			create_atomic_unification(OFV, var(NFV),
> +		create_atomic_unification(HoistedVar, var(OldVar),
>  				Context, MainCtxt, SubCtxt, Goal),

I suspect that indentation change wasn't deliberate.

> +:- pred update_existential_data_structures(
> +	assoc_list(prog_var)::in, list(assoc_list(prog_var))::in,
> +	cse_info::in, cse_info::out) is det.
> +
> +update_existential_data_structures(FirstOldNew, LaterOldNews,
> +		CseInfo0, CseInfo) :-
> +	list__condense(LaterOldNews, LaterOldNew),
> +	list__append(FirstOldNew, LaterOldNew, OldNew),
> +	map__from_assoc_list(OldNew, OldNewMap),
> +	map__from_assoc_list(FirstOldNew, FirstOldNewMap),
> +
> +	TypeInfoVarMap0 = CseInfo0 ^ type_info_varmap,
> +	TypeClassInfoVarMap0 = CseInfo0 ^ typeclass_info_varmap,
> +	VarTypes0 = CseInfo0 ^ vartypes,
> +
> +	map__to_assoc_list(TypeInfoVarMap0, TypeInfoVar0),

`TypeInfoVar0' isn't a great name --- it is an assoc list, not a var.

> +:- pred reconstruct_typeclass_info_varmap(map(prog_var, prog_var)::in,
> +	map(tvar, tvar)::in, pair(class_constraint, prog_var)::in,
> +	typeclass_info_varmap::in, typeclass_info_varmap::out) is det.
> +
> +reconstruct_typeclass_info_varmap(OldNewMap, TvarSub,
> +		Constraint0 - TypeClassInfoVar0,
> +		TypeClassInfoVarMap0, TypeClassInfoVarMap) :-
> +	Constraint0 = constraint(ClassName, Types0),
> +	list__map(apply_tvar_rename(TvarSub), Types0, Types),
> +	Constraint = constraint(ClassName, Types),

Rather than deconstructing the constraint and applying the renaming, you
should use type_util:apply_variable_renaming_to_constraint.

> +:- pred apply_tvar_rename(map(tvar, tvar)::in,
> +	(type)::in, (type)::out) is det.
> +
> +apply_tvar_rename(TvarSub, Type0, Type) :-
> +	Type = term__apply_variable_renaming(Type0, TvarSub).

These two should probably go in type_util.m too.

Otherwise, this diff looks fine to me.

Thanks, Zoltan.


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