[m-dev.] Tabling round 2 [1/3]

[Fergus Henderson]

On 13-Mar-1998, Oliver Hutchison <ohutch at students.cs.mu.oz.au> wrote:
> diff -u -r1.49 det_report.m
> --- det_report.m	1998/03/03 17:34:04	1.49
> +++ det_report.m	1998/03/10 01:50:10
...
> +		{ proc_info_context(ProcInfo0, Context) },
> +		prog_out__write_context(Context),
> +		{ eval_method_to_string(EvalMethod0, EvalMethodS) },
> +		io__write_string("Error: `pragma "),
> +		io__write_string(EvalMethodS),
> +		io__write_string("' declaration not allowed for procedure\n"),
> +		prog_out__write_context(Context),
> +		io__write_string("with determinism `"),
> +		mercury_output_det(InferredDetism),
> +		io__write_string("'.\n"), 

The second and subsequent lines of error messages should be indented
by two spaces:

	s/with/  with/
	       ^^

> +"\tThe pragma requested is only valid for the folowing determinism(s): \n"),

s/ \n/\n/
  ^

> Index: compiler/hlds_pred.m
...
> +	% The evaluation method that should be used for a pred
> +
> +:- type eval_method	--->	eval_normal		% normal mercury 
> +							% evaluation
> +			;	eval_memo		% memoing evaluation
> +			;	eval_loop_check		% memoing + loop check

The two comments above are wrong.

> +			;	if_valid(eval_method).	% ignore the eval model
> +							% if the detism of the 
> +							% proc is not valid for 
> +							% the eval model

Is the `if_valid' eval_method ever actually used?
If so, what is it used for?

> +	% XXX : We can't really use the stratification check, because it is 
> +	% too conservative.
> +eval_method_need_stratification(_) :-
> +	semidet_fail.	

For `eval_minimal', it would be nice to do a stratification check.
The current stratification check is indeed too conservative.
But it might be helpful if it just warned about those cases where
the code was known to be statically non-stratified, rather than about
the cases where the code was not known to be stratified.

> +module_add_pragma_tabled(EvalMethod, PredName, Arity, MaybePredOrFunc, 
> +		MaybeModes,  Context, ModuleInfo0, ModuleInfo) --> 
> +	{ module_info_get_predicate_table(ModuleInfo0, PredicateTable0) }, 
> + 	{ eval_method_to_string(EvalMethod, EvalMethodS) },
> +		
> +	% Find out if we are tabling a predicate or a function 
> +	(
> +		{ MaybePredOrFunc = yes(PredOrFunc0) }
> +	->
> +		{ PredOrFunc = PredOrFunc0 },
> +
> +			% Lookup the pred declaration in the predicate table.
> +			% (If it's not there, print an error message and insert
> +			% a dummy declaration for the predicate.) 
> +		(
> +			{ predicate_table_search_pf_sym_arity(PredicateTable0,
> +				PredOrFunc, PredName, Arity, [PredId1]) }
> +		->
> +			{ PredId = PredId1 },
> +			{ ModuleInfo1 = ModuleInfo0 }	
> +		;
> +			{ module_info_name(ModuleInfo0, ModuleName) },
> +			{ string__format("pragma (%s)", [s(EvalMethodS)], 
> +				Message1) },
> +			maybe_undefined_pred_error(PredName, Arity, 
> +				PredOrFunc, Context, Message1),
> +			{ preds_add_implicit(PredicateTable0,
> +				ModuleName, PredName, Arity, Context,
> +				PredOrFunc, PredId, PredicateTable1) },
> +			{ module_info_set_predicate_table(ModuleInfo0,
> +				PredicateTable1, ModuleInfo1) }
> +		),
> +		module_add_pragma_tabled_2(EvalMethod, PredName, Arity,
> +			MaybePredOrFunc, MaybeModes, Context, PredId,
> +			ModuleInfo1, ModuleInfo)

You still haven't fixed that to handle the case where
search_pf_sym_arity returns multiple answers in the way I said.

> +:- pred module_add_pragma_tabled_2(eval_method, sym_name, int, 
> +		maybe(pred_or_func), maybe(list(mode)), term__context,
> +		pred_id, module_info, module_info, io__state, io__state).
> +:- mode module_add_pragma_tabled_2(in, in, in, in, in, in, in, in, out,
> +		di, uo) is det.
> +
> +module_add_pragma_tabled_2(EvalMethod, PredName, Arity0, MaybePredOrFunc, 
> +		MaybeModes, Context, PredId, ModuleInfo0, ModuleInfo) -->
> +	{ module_info_get_predicate_table(ModuleInfo0, PredicateTable) }, 
> + 	
> +	% Find out if we are tabling a predicate or a function 
> +	(
> +		{ MaybePredOrFunc = yes(PredOrFunc0) }
> +	->
> +		{ PredOrFunc = PredOrFunc0 }
> +	;
> +		{ predicate_table_get_preds(PredicateTable, Preds0) },
> +		{ map__lookup(Preds0, PredId, PredInfo0) },
> +		{ pred_info_get_is_pred_or_func(PredInfo0, PredOrFunc) }
> +	),
> +	(
> +		{ PredOrFunc = predicate },
> +		{ Arity = Arity0 }
> +	;
> +		{ PredOrFunc = function },
> +		{ Arity is Arity0 + 1 }
> +	),
> +		
> +		% print out a progress message
> +	{ eval_method_to_string(EvalMethod, EvalMethodS) },
> +	globals__io_lookup_bool_option(very_verbose, VeryVerbose),
> +	( 
> +		{ VeryVerbose = yes }
> +	->
> +		io__write_string("% Processing `:- pragma "),
> +		io__write_string(EvalMethodS),
> +		io__write_string("' for "),
> +		hlds_out__write_call_id(PredOrFunc, PredName/Arity),
> +		io__write_string("...\n")
> +	;
> +		[]
> +	),
> +		
> +		% Lookup the pred_info for this pred,
> +	{ predicate_table_get_preds(PredicateTable, Preds1) },
> +	{ map__lookup(Preds1, PredId, PredInfo1) },

Rather than duplicating that code, it would
be better to just move those two lines to the top
of this predicate, and delete the duplicate copy above.

modes.m:
> +:- pred proc_check_eval_methods(list(proc_id), pred_id, module_info, 
> +		module_info, io__state, io__state).
> +:- mode proc_check_eval_methods(in, in, in, out, di, uo) is det.
> +
> +proc_check_eval_methods([], _, M, M) --> [].
> +proc_check_eval_methods([ProcId|Rest], PredId, ModuleInfo0, ModuleInfo) --> 
> +	{ module_info_pred_proc_info(ModuleInfo0, PredId, ProcId, 
> +		_, ProcInfo) },
> +	{ proc_info_eval_method(ProcInfo, EvalMethod) },
> +	( { EvalMethod \= eval_normal } ->
> +		{ proc_info_argmodes(ProcInfo, Modes) },
> +		( 
> +			\+ { only_fully_in_out_nonunique_modes(Modes, 
> +				ModuleInfo0) } 
> +		->
> +			{ proc_info_context(ProcInfo, Context) },
> +			prog_out__write_context(Context),
> +			{ eval_method_to_string(EvalMethod, EvalMethodS) },
> +			io__write_string("Error : `pragma "),
> +			io__write_string(EvalMethodS),
> +			io__write_string(
> +"' declaration not allowed for procedure with\n"),
> +			prog_out__write_context(Context),
> +			io__write_string(
> +"unique/partially instantiated modes.\n"), 
> +			globals__io_lookup_bool_option(verbose_errors, 
> +				VerboseErrors),
> +			( { VerboseErrors = yes } ->
> +				io__write_string(
> +"	Tabling of predicates/functions with unique modes is not allowed
> +	as this would lead to a copying of the unique arguments which 
> +	would result in them no longer being unique.
> +	Tabling of predicates/functions with partially instantiated modes
> +	is not currently implemented.\n")
> +			;
> +				[]
> +			),
> +			{ module_info_incr_errors(ModuleInfo0, ModuleInfo1) }
> +		;
> +			{ ModuleInfo1 = ModuleInfo0 }	
> +		)
> +	;
> +		{ ModuleInfo1 = ModuleInfo0 }	
> +		
> +	),
> +	proc_check_eval_methods(Rest, PredId, ModuleInfo1, ModuleInfo).
> +
> +:- pred only_fully_in_out_nonunique_modes(list(mode), module_info).
> +:- mode only_fully_in_out_nonunique_modes(in, in) is semidet.
> +
> +only_fully_in_out_nonunique_modes([], _).
> +only_fully_in_out_nonunique_modes([Mode|Rest], ModuleInfo) :-
> +	mode_get_insts(ModuleInfo, Mode, InitialInst, FinalInst),
> +	inst_is_not_partly_unique(ModuleInfo, InitialInst),
> +	inst_is_not_partly_unique(ModuleInfo, FinalInst),
> +	(
> +		inst_is_ground(ModuleInfo, InitialInst)	
> +	;
> +		inst_is_free(ModuleInfo, InitialInst),
> +		inst_is_ground(ModuleInfo, FinalInst)
> +	),
> +	only_fully_in_out_nonunique_modes(Rest, ModuleInfo).

It would be better to test for uniqueness
and for partially instantiated modes seperately, and to
issue separate error messages for each of them.
This would result in better error messages.

Also you didn't respond to my query about allowing `free->free' modes.

> +		% 
> +		% Dont count procs using minimal evaluation
> +		%
> +		\+ proc_info_eval_method(ProcInfo, eval_minimal)	
> +	->	

s/Dont/Don't/

Also it would be helpful to include a brief comment explaining why.

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