for review: make HLDS mostly well-moded

[Fergus Henderson]

On 09-Feb-1998, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> compiler/unique_modes.m
> compiler/hlds_pred.m
> 	Added proc_info_inferred_never_succeeds, which checks whether
> 	the procedure never succeeds according to the inferred determinism.
> 	Use it during unique_mode analysis instead of proc_info_never_succeeds,
> 	which uses the declared determinism.

Why?

This changes which programs are considered legal programs.
Some programs that would previously have been mode-incorrect will become
mode-correct.  At very least some rationale is needed.

> compiler/special_pred.m
> library/mercury_builtin.m
> 	Make the uniqueness of the comparison_result argument
> 	of builtin_compare_* and the automatically generated
> 	comparison procedures match that of compare/3. Unique mode 
> 	errors will still be introduced if any of the unique modes
> 	of compare/3 are used.

Could you explain this in a bit more detail?

> tests/valid/unreachable_code.m
> 	Add a test case for a bogus higher-order unification
> 	mode error in unreachable code.

Hmm, there were several things described as "bug fixes" in the log message --
does this test case test for all of them?  I'd like to see a regression
test for each bug you fixed, if possible.

>  call_gen__generate_class_method_call(_OuterCodeModel, TCVar, MethodNum, Args,
>  		Types, Modes, Det, GoalInfo, Code) -->
>  	{ determinism_to_code_model(Det, InnerCodeModel) },
>  	code_info__get_globals(Globals),
>  	code_info__get_module_info(ModuleInfo),
> +
> +	% XXX must be compact
>  	{ globals__get_args_method(Globals, ArgsMethod) },
>  	{ make_arg_infos(ArgsMethod, Types, Modes, InnerCodeModel, ModuleInfo,
>  		ArgInfo) },

Good point.

The code should check this and abort if it is not true.
(Either here, or in runtime/mercury_ho_call.c.)

> -find_follow_vars_in_goal_2(unify(A,B,C,D,E), ArgsMethod, _ModuleInfo,
> +find_follow_vars_in_goal_2(unify(A,B,C,D,E), _ModuleInfo,
>  		FollowVars0, unify(A,B,C,D,E), FollowVars) :-
>  	(
> -		B = var(BVar),
> -		D = complicated_unify(_Mode, CanFail)
> -	->
> -		determinism_components(Det, CanFail, at_most_one),
> -		determinism_to_code_model(Det, CodeModel),
> -		arg_info__unify_arg_info(ArgsMethod, CodeModel, ArgInfo),
> -		find_follow_vars_from_arginfo(ArgInfo, [A, BVar], FollowVars)
> -	;

I didn't see anything about that in the log message.
What's the rationale for this change?

hlds_out.m:
> +	io__write_string("% args method: "),
> +	hlds_out__write_args_method(ArgsMethod),
> +	io__nl,
...
> +:- pred hlds_out__write_args_method(args_method, io__state, io__state).
> +:- mode hlds_out__write_args_method(in, di, uo) is det.
> +
> +hlds_out__write_args_method(simple) -->
> +	io__write_string("simple").
> +hlds_out__write_args_method(compact) -->
> +	io__write_string("compact").

It might be simpler to just use io__write, although I suppose now that
it's written it doesn't matter much either way.

hlds_goal.m:
> +			args_method
> +					% The args_method to be used for
> +					% the procedure. Usually this will
> +					% be set to the value of the --args
> +					% option stored in the globals. 
> +					% lambda.m will set this field to
> +					% compact for procedures it creates
> +					% which must be directly callable by
> +					% a higher_order_call goal.

I suggest s/compact/`compact'/

> +proc_info_inferred_never_succeeds(PredInfo, ProcId, ProcInfo, Result) :-
> +	(
> +		% We don't infer determinism for imported procedures
> +		% or for class_method predicates.
> +		(
> +			pred_info_is_imported(PredInfo)
> +		;
> +			pred_info_is_pseudo_imported(PredInfo),
> +			hlds_pred__in_in_unification_proc_id(ProcId)
> +		;
> +			pred_info_get_markers(PredInfo, Markers),
> +			check_marker(Markers, class_method)
> +		)
> +	->
> +		proc_info_interface_determinism(ProcInfo, Determinism)
> +	;	
> +		proc_info_inferred_determinism(ProcInfo, Determinism)
> +	),

Hmm, in the cases of imported procedures or class methods,
won't proc_info_inferred_determinism just return the same as
proc_info_interface_determinism anyway?

In other words, couldn't you just replace the above code with
a single call to `proc_info_inferred_determinism'?

> @@ -280,6 +275,14 @@
>  	( 
>  		LambdaGoal = call(PredId0, ProcId0, CallVars,
>  					_, _, PredName0) - _,
> +		module_info_pred_proc_info(ModuleInfo0, PredId0, ProcId0, _,
> +			Call_ProcInfo),
> +
> +			% check that this procedure uses an args_method which 
> +			% is always directly higher-order callable.
> +		proc_info_args_method(Call_ProcInfo, Call_ArgsMethod),
> +		arg_info__ho_callable_args_method(Call_ArgsMethod),

Hmm.  `arg_info__ho_callable_args_method' is used for two purposes --
here to check whether an args_method is usable for h.o. calls,
elsewhere to pick an args_method to use for h.o. calls.
For full generality, the latter ought to be achieved using
a different predicate called say arg_info__ho_call_args_method,
preferably taking the globals as an argument.
And for maintainability, the former should be done using a det
predicate which returns a bool.
That way, it would work if we add a new args_method which is
usable for h.o. calls.

modecheck_unify.m:
> @@ -826,22 +842,30 @@
>  			determinism_components(Det, CanFail, _),
>  			UniMode = ((IX - IY) -> (FX - FY)),
>  			Unification = complicated_unify(UniMode, CanFail),
> +			mode_info_get_instmap(ModeInfo0, InstMap0),
>  			(
>  				type_is_higher_order(Type, PredOrFunc, _)
>  			->
> -				% we do not want to report this as an error
> +				% We do not want to report this as an error
>  				% if it occurs in a compiler-generated
>  				% predicate - instead, we delay the error
>  				% until runtime so that it only occurs if
> -				% the compiler-generated predicate gets called
> +				% the compiler-generated predicate gets called.
> +				% We don't report an error if the instmap is
> +				% unreachable since not_reached counts
> +				% as bound.

"since not_reached counts as bound" is the reason why this
code might get executed if the instmap is unreachable,
it's not the reason why you don't report an error.

Oh, you will have a problem with the following optimization
in modecheck_unify.m:

	%
	% Optimize away construction of unused terms by
	% replacing the unification with `true'.  Optimize
	% away unifications which always fail by replacing
	% them with `fail'.
	%
	(
		Unification = construct(ConstructTarget, _, _, _),
		mode_info_var_is_live(ModeInfo, ConstructTarget, dead)
	->
		Goal = conj([])
	;
		Det = failure
	->
		% This optimisation is safe because the only way that
		% we can analyse a unification as having no solutions
		% is that the unification always fails.
		%
		% Unifying two preds is not erroneous as far as the
		% mode checker is concerned, but a mode _error_.
		map__init(Empty),
		Goal = disj([], Empty)
	;

The problem is that by rerunning mode analysis after inlining
you have violated the assumption documented there (the one starting
with "This optimisation is safe because ...").
This optimization might do the wrong thing in the case
of user-defined equality predicates, e.g. optimizing away
a call to error/1.

> @@ -916,15 +942,47 @@
>  	;
>  		% the real cons_id will be computed by polymorphism.m;
>  		% we just put in a dummy one for now
> -		list__length(ArgVars, Arity),
>  		ConsId = cons(unqualified("__LambdaGoal__"), Arity)
>  	),
>  	mode_info_get_module_info(ModeInfo0, ModuleInfo),
>  	mode_util__modes_to_uni_modes(ArgModes0, ArgModes0,
>  						ModuleInfo, ArgModes),
> +	mode_info_get_instmap(ModeInfo0, InstMap),
> +		% If the instmap is unreachable, the code will 
> +		% get pruned away, so don't report an error.
>  	(
> +		instmap__is_unreachable(InstMap)
> +	->
> +		ModeInfo = ModeInfo0,
> +		RHS = RHS0,
> +		Unification = construct(X, ConsId, ArgVars, ArgModes)
> +	;
>  		mode_is_output(ModuleInfo, ModeOfX)
>  	->
> +		( 
> +			% lambda expressions must already have been expanded
> +			% if a pred_const is present.
> +			ConsId = pred_const(PredId, ProcId)
> +		->
> +			( 
> +				RHS0 = lambda_goal(_, _, _, _, _, Goal),
> +				Goal = call(PredId, ProcId, _, _, _, _) - _
> +			->
> +				module_info_pred_info(ModuleInfo,
> +					PredId, PredInfo),
> +				pred_info_module(PredInfo, PredModule),
> +				pred_info_name(PredInfo, PredName),
> +				RHS = functor(
> +					cons(qualified(PredModule, PredName),
> +						Arity),
> +					ArgVars)	
> +			;
> +				error("categorize_unify_var_lambda - \
> +					reintroduced lambda goal")
> +			)
> +		;
> +			RHS = RHS0
> +		),
>  		Unification = construct(X, ConsId, ArgVars, ArgModes),
>  		ModeInfo = ModeInfo0
>  	;

I found this code quite confusing.  Perhaps some better documentation
would help.  What is the code trying to do?

> --- mercury_ho_call.c	1997/12/20 11:11:30	1.5
> +++ mercury_ho_call.c	1998/02/07 13:52:37
> @@ -136,22 +136,9 @@
>  	*/
>  Define_label(det_closure_return);
>  {
> -	int	i, num_in_args, num_out_args;
> -
>  	MR_succip = pop(); /* restore succip */
> -	num_in_args = pop(); /* restore the input arg counter */
> -	num_out_args = pop(); /* restore the ouput arg counter */
> -
> -#ifdef	COMPACT_ARGS
> -#else
> -	save_registers();
> -
> -	for (i = 1; i <= num_out_args; i++) {
> -		virtual_reg(i) = virtual_reg(i+num_in_args);
> -	}
> -
> -	restore_registers();
> -#endif
> +	pop(); /* restore the input arg counter */
> +	pop(); /* restore the ouput arg counter */
>  
>  	proceed();
>  }

No point pushing the data in the first place if you're not going
to look at it when you pop it.

If you assume COMPACT_ARGS, then you can optimize this to use a
tailcall() instead of a call(), and dispense with det_closure_return
altogether.  Similarly for semidet_ and nondet_closure_return.


Apart from those points raised above, it looks good.  Thanks, Simon.
Can you please make the next diff relative to this one?

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