[m-rev.] for review: stack sloty optimization, part 2

[Fergus Henderson]

On 09-Mar-2002, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Index: compiler/follow_code.m
...
> +move_follow_code_select([Goal | Goals], FollowGoals, RestGoals) :-
> +	% Goal = _ - GoalInfo,
> +	% \+ goal_info_has_feature(GoalInfo, (impure))

There should be a comment here explaining why this code is
commented out.

> +++ compiler/goal_util.m	22 Feb 2002 05:17:54 -0000
> @@ -6,8 +6,19 @@
>  
>  % Main author: conway.
>  %
> -% This module provides various utility procedures for manipulating HLDS goals,
> -% e.g. some functionality for renaming variables in goals.
> +% This module provides some functionality for renaming variables in goals.

This change is misleading, since the module provides other functionality
too.

> +% The predicates rename_var* take a structure and a mapping from var -> var
> +% and apply that translation. If a var in the input structure does not
> +% occur as a key in the mapping, then the variable is left unsubstituted.
> +
> +% goal_util__create_variables takes a list of variables, a varset an
> +% initial translation mapping and an initial mapping from variable to
> +% type, and creates new instances of each of the source variables in the
> +% translation mapping, adding the new variable to the type mapping and
> +% updating the varset. The type for each new variable is found by looking
> +% in the type map given in the 5th argument - the last input.
> +% (This interface will not easily admit uniqueness in the type map for this
> +% reason - such is the sacrifice for generality.)

There's no point duplicating those comments here at the top of the module;
those comments already occur in front of the predicates that they are
documenting, and that is where they belong.

>  :- mode convert_dump_alias(in, out) is semidet.
>  
> +convert_dump_alias("osv", "bcdglmnpruvP").

A comment `% for debugging optimize-saved-vars' here would help.

> Index: compiler/hlds_llds.m
...
> +:- type need_across_call
> +	--->	need_across_call(
> +	 		call_forward_vars	:: set(prog_var),
> +	 		call_resume_vars	:: set(prog_var),
> +	 		call_nondet_vars	:: set(prog_var)
> +		).
> +
> +:- type need_in_resume
> +	--->	need_in_resume(
> +			resume_vars_on_stack	:: bool,
> +	 		resume_resume_vars	:: set(prog_var),
> +	 		resume_nondet_vars	:: set(prog_var)
> +		).
> +
> +:- type need_in_par_conj
> +	--->	need_in_par_conj(
> +	 		par_conj_engine_vars	:: set(prog_var)
> +		).

There should be some documentation for these types,
or a pointer to where to find it.

> +:- pred goal_info_initialize_liveness_info(hlds_goal_info::in,
> +	set(prog_var)::in, set(prog_var)::in, set(prog_var)::in,
> +	set(prog_var)::in, resume_point::in, hlds_goal_info::out) is det.

The meaning of the arguments of this procedure should be documented.

> +:- pred rename_vars_in_llds_code_gen_info(llds_code_gen_details::in,
> +	bool::in, map(prog_var, prog_var)::in, llds_code_gen_details::out)
> +	is det.

This predicate should be documented,
especially the meaning of the bool argument.

> +:- implementation.
....
> +:- type maybe_need
> +	--->	no_need
> +	;	need_call(need_across_call)
> +	;	need_resume(need_in_resume)
> +	;	need_par_conj(need_in_par_conj).

This type should be documented.

> +:- type llds_code_gen_details --->
> +	llds_code_gen_details(
> +		pre_births		:: set(prog_var),
> +		post_births		:: set(prog_var),
> +		pre_deaths		:: set(prog_var),
> +		post_deaths		:: set(prog_var),
> +			% All four of these fields are computed by liveness.m.
> +			% For atomic goals, the post-deadness should be applied
> +			% _before_ the goal.

> +		follow_vars		:: maybe(follow_vars),
> +			% Advisory information about where variables
> +			% ought to be put next. The legal range
> +			% includes the nonexistent register r(-1),
> +			% which indicates any available register.

It would be better to just say "see the documentation for the
follow_vars type", rather than duplicating it here.

> +		maybe_need		:: maybe_need
> +			% These three fields are filled in during the
> +			% stackvars pass. They are not meaningful before then,
> +			% and should therefore contain `no'.

These three fields?  I only see one.  I think maybe there were originally
three fields but later you realized that they could be combined into one.
This sentence, and the documentation which follows it, should be updated
to reflect that change.

Otherwise this part of the diff looks fine.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------