[m-dev.] for review: new method of handling failures, part 1 of 6

[Fergus Henderson]

Here's my comments on part 1 [code_info.m].

Firstly, this version is a major improvement!
The code is much more modular and much better encapsulated.

Some of these comments below may relate to existing code that was not
modified in this change.  But this may be a good opportunity to revisit
that code anyway.

> :- pred code_info__get_cell_count(int, code_info, code_info).
> :- mode code_info__get_cell_count(out, in, out) is det.

What's the cell count?  What kind of cells is it counting?
Please add some documentation here.

> :- pred code_info__get_headvars(list(var), code_info, code_info).
> :- mode code_info__get_headvars(out, in, out) is det.

Please add a comment for this pred:

 	% Get the list of head variables for the current procedure.

> :- pred code_info__variable_to_string(var, string, code_info, code_info).
> :- mode code_info__variable_to_string(in, out, in, out) is det.
> 
> :- pred code_info__apply_instmap_delta(instmap_delta, code_info, code_info).
> :- mode code_info__apply_instmap_delta(in, in, out) is det.

Brief comments here would be helpful.

> 	% Create a code address which holds the address of the specified
> 	% procedure.
> 	% The fourth argument should be `no' if the the caller wants the
> 	% returned address to be valid from everywhere in the program.
> 	% If being valid from within the current procedure is enough,
> 	% this argument should be `yes' wrapped around the value of the
> 	% --procs-per-c-function option and the current procedure id.
> 	% Using an address that is only valid from within the current
> 	% procedure may make jumps more efficient.
> 	% XXX
> :- pred code_info__make_entry_label(module_info, pred_id, proc_id, bool,
> 	code_addr, code_info, code_info).
> :- mode code_info__make_entry_label(in, in, in, in, out, in, out) is det.

What's the XXX for?

> :- pred code_info__succip_is_used(code_info, code_info).
> :- mode code_info__succip_is_used(in, out) is det.

A brief comment for this one would be helpful.

> code_info__get_headvars(HeadVars) -->
> 	code_info__get_module_info(ModuleInfo),
> 	code_info__get_pred_id(PredId),
> 	code_info__get_proc_id(ProcId),
> 	{ module_info_preds(ModuleInfo, Preds) },
> 	{ map__lookup(Preds, PredId, PredInfo) },
> 	{ pred_info_procedures(PredInfo, Procs) },
> 	{ map__lookup(Procs, ProcId, ProcInfo) },
> 	{ proc_info_headvars(ProcInfo, HeadVars) }.
...
> code_info__get_pred_proc_arginfo(PredId, ProcId, ArgInfo) -->
> 	code_info__get_module_info(ModuleInfo),
> 	{ module_info_pred_proc_info(ModuleInfo, PredId, ProcId, _, ProcInfo) },
> 	{ proc_info_arg_info(ProcInfo, ArgInfo) }.

code_info__get_headvars should use module_info_pred_proc_info,
just like get_pred_proc_arginfo does.

> code_info__apply_instmap_delta(Delta) -->
> 	code_info__get_instmap(InstMap0),
> 	{ instmap__apply_instmap_delta(InstMap0, Delta, InstMap) },
> 	code_info__set_instmap(InstMap).

Why is this needed -- when do we need to update the instmap
other than in code_info__post_goal_update?

If it is not used elsewhere, it should not be exported.

> :- pred code_info__undo_disj_hijack(disj_hijack_info::in,
> 	code_tree::out, code_info::in, code_info::out) is det.

Please document that predicate.

> :- type ite_hijack_info.
> 
> :- pred code_info__prepare_for_ite_hijack(code_model::in,
> 	ite_hijack_info::out, code_tree::out,
> 	code_info::in, code_info::out) is det.
> 
> :- pred code_info__maybe_push_temp_frame(code_model::in, bool::in,
> 	ite_hijack_info::in, lval::out, code_tree::out,
> 	code_info::in, code_info::out) is det.
> 
> :- pred code_info__ite_enter_then(ite_hijack_info::in, lval::in,
> 	code_tree::out, code_tree::out, code_info::in, code_info::out) is det.
> 
> :- type simple_neg_info.
> 
> :- pred code_info__enter_simple_neg(set(var)::in, hlds_goal_info::in, 
> 	simple_neg_info::out, code_info::in, code_info::out) is det.
> 
> :- pred code_info__leave_simple_neg(hlds_goal_info::in, simple_neg_info::in,
> 	code_info::in, code_info::out) is det.

Please document these.

> 	% Put the given resume point into effect, by pushing it on to
> 	% the resume point stack, and if necessary overriding the redoip
> 	% of the top frame.
> 
> :- pred code_info__effect_resume_point(resume_point_info::in, code_model::in,
> 	code_tree::out, code_info::in, code_info::out) is det.

The comment would be clearer, IMHO, if it started with "Generate code to ".

> :- pred code_info__generate_failure(code_tree::out,
> 	code_info::in, code_info::out) is det.
>
> :- pred code_info__fail_if_rval_is_false(rval::in, code_tree::out,
> 	code_info::in, code_info::out) is det.
> 
> :- pred code_info__failure_is_direct_branch(code_addr::out,
> 	code_info::in, code_info::out) is semidet.
> 
> :- pred code_info__may_use_nondet_tailcall(bool::out,
> 	code_info::in, code_info::out) is det.

The names here are quite explanatory, but even so I think some
brief comments would be helpful.

> :- pred code_info__generate_det_pre_commit(det_commit_info::out,
> 	code_tree::out, code_info::in, code_info::out) is det.

For consistency with elsewhere, you might want to rename that as
code_info__prepare_for_det_commit (and likewise for the semidet one).

(Am I right that this would be consistent with the other uses of
`prepare_for'?)

> :- type resume_map		==	map(var, set(rval)).

A comment here would be helpful.

> :- type resume_point_info
> 	--->	orig_only(resume_map, code_addr)
> 	;	stack_only(resume_map, code_addr)
> 	;	orig_and_stack(resume_map, code_addr, resume_map, code_addr)
> 	;	stack_and_orig(resume_map, code_addr, resume_map, code_addr).
> 
> :- type fail_info
> 	--->	fail_info(
> 			stack(resume_point_info),
> 			resume_point_known,
> 			curfr_vs_maxfr
> 		).

It might be more readable if you order the type definitions here
in top-down order rather than bottom-up.

> code_info__enter_simple_neg(ResumeVars, GoalInfo, FailInfo0) -->
> 	code_info__get_fail_info(FailInfo0),
> 		% The only reason why we push a resume point at all
> 		% is to protect the variables in ResumeVars from becoming
> 		% unknown; by including them in the domain of the resume point,
> 		% we guarantee that the will become zombies instead of unknown
> 		% if they die in the pre- or post-goal updates.
> 		% Therefore the only part of ResumePoint that matters
> 		% is the set of variables in the resume map; the other
> 		% parts of ResumePoint (the locations, the code address)
> 		% will not be referenced.

What's a zombie?  What's the difference between a zombie and unknown?

> :- pred code_info__match_resume_loc(resume_map::in, resume_map::in) is semidet.
> 
> code_info__match_resume_loc(Map, Locations0) :-
> 	map__keys(Map, KeyList),
> 	set__list_to_set(KeyList, Keys),
> 	map__select(Locations0, Keys, Locations),
> 	map__to_assoc_list(Locations, List),
> 	\+ (
> 		list__member(Thingy, List),
> 		\+ (
> 			Thingy = Var - Actual,
> 			map__search(Map, Var, Rvals),
> 			set__subset(Rvals, Actual)
> 		)
> 	).

`Thingy' is a rather non-descriptive variable name.
I suggest unfolding the unification `Thingy = Var - Actual' in
the call to `list__member'.

I also suggest using `all' and `=>' rather than nested '\+'.

	all [Var, Actual] (
		list__member(Var - Actual, List) => (
			map__search(Map, Var, Rvals),
			set__subset(Rvals, Actual)
		)
	)

Previously we were avoiding this because of efficiency problems in
NU-Prolog (caused by NU-Prolog's runtime groundness checks),
but NU-Prolog compatibility is no longer an issue.
`all' should work fine in SICstus Prolog.

> :- pred extract_label_from_code_addr(code_addr::in, label::out) is det.
> 
> extract_label_from_code_addr(CodeAddr, Label) :-
> 	( CodeAddr = label(Label0) ->
> 		Label = Label0
> 	;
> 		error("code_info__generate_resume_setup: non-label!")
> 	).

s/code_info__generate_resume_setup/extract_label_from_code_addr/

> %---------------------------------------------------------------------------%
> 
> 	% Submodule to deal with liveness issues.
>
> :- interface.
> 
> :- pred code_info__get_known_variables(list(var), code_info, code_info).
> :- mode code_info__get_known_variables(out, in, out) is det.
> 
> :- pred code_info__variable_is_forward_live(var, code_info, code_info).
> :- mode code_info__variable_is_forward_live(in, in, out) is semidet.
> 
> :- pred code_info__make_vars_forward_dead(set(var), code_info, code_info).
> :- mode code_info__make_vars_forward_dead(in, in, out) is det.
> 
> :- pred code_info__pickup_zombies(set(var), code_info, code_info).
> :- mode code_info__pickup_zombies(out, in, out) is det.

It would be helpful to put a pointer to compiler/notes/allocation.html here.

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