[m-rev.] for review: centralize compiler's knowledge of __Unify__ etc names

[Zoltan Somogyi]

On 27-May-2003, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> On 24-May-2003, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> > compiler/hlds_pred.m:
> > 	Add a field to pred_infos that says whether the predicate is a unify,
> > 	compare or index predicate, and if so, for which type constructor.
> > 	Code that used to test the predicate's name for __Unify__ etc now
> > 	tests this field instead. Similarly the code that used to employ
> > 	devious tricks to find out the type the unify/compare/index predicate
> > 	is for.
> 
> Why use a data field rather than a function (which may be accessed
> via similar syntax to a data field)?

Because the devious tricks one would need in the implementation of that
function are some of the things that I am trying to get rid of.

> Using a data field here will increase the compiler's memory usage,
> increase GC costs, and reduce locality,

If this is a concern, the right approach is to separate the fields of
pred_infos (and proc_infos) into the ones that are often updated and the ones
that are mostly static (as we do with module_infos). Doing this properly
would require using profiling to determine the numbers of times the setter
predicates are invoked, and would be better done as a separate change.

> quite possibly to a significant
> degree.

I think that is very unlikely. There are many fewer pred_infos than e.g.
goal_infos, and they are mostly stable after semantic checking. In any case,
the code we used to have to find out whether a predicate was a unify, compare
or index predicate, and if so for which type, used to turn over memory too.

> > compiler/hlds_out.m:
> > 	Don't refer to the 'type' that a unify, compare or index predicate
> > 	is for; refer to the type *constructor*.
> 
> ...
> > -		{ special_pred_description(Kind, Descr) },
> > +		{ special_pred_description(SpecialId, Descr) },
> >  		io__write_string(Descr),
> > -		io__write_string(" for type "),
> > -		{ pred_info_arg_types(PredInfo, TVarSet, _ExistQVars,
> > -			ArgTypes) },
> > -		( { special_pred_get_type(Name, ArgTypes, Type) } ->
> > -			mercury_output_term(Type, TVarSet, no)
> > -		;
> > -			{ error("special_pred_get_type failed!") }
> > -		)
> > +		io__write_string(" for type constructor "),
> > +		hlds_out__write_type_name(TypeCtor)
> 
> This does not seem like an improvement,
> since now the message doesn't specify the arity.

Yes, it does: that is done by write_type_name. See the update of the test case.

> > +++ compiler/magic_util.m	24 May 2003 10:11:27 -0000
> > @@ -654,11 +654,11 @@
> >  		% It had better be an in-in unification, since Aditi
> >  		% relations cannot have non-ground arguments. This is 
> >  		% checked elsewhere.
> > -		% XXX __Unify__/2 needs to be special cased in rl_exprn.m 
> > -		% because we don't add the type_info arguments.
> > +		% XXX unification predicates need to be special cased
> > +		% in rl_exprn.m because we don't add the type_info arguments.
> >  
> >  		hlds_pred__in_in_unification_proc_id(UniProcId),
> > -		SymName = unqualified("__Unify__"),
> > +		XXX SymName = unqualified("__Unify__"),
> >  		ArgVars = [Var, OutputVar],
> >  		Test = call(UniPredId, UniProcId, ArgVars, not_builtin,
> >  			no, SymName) - GoalInfo
> 
> Hmm, isn't that XXX a syntax error?
> It looks like the diff that you posted hasn't been compiled.

Yes, it has. The entire section of code above is within /* */ comments,
and I deliberately made the code syntactically incorrect so that if someone
removes the /* */, they will know they need to fix the code.

BTW, this is the reason why I don't like /* */ comments in Mercury code:
you can't tell at a glance whether a piece of code is live or not.

> Otherwise, that looks fine.  But the efficiency issue that I mentioned
> at the top regarding the extra data field is an important one, which
> should be addressed before this is committed.

I don't think there is any significant efficiency issue.

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