[m-dev.] for review: retry across I/O

[Fergus Henderson]

On 06-Nov-2000, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
>  eval_method_requires_ground_args(eval_normal) = no.
>  eval_method_requires_ground_args(eval_loop_check) = yes.
> +eval_method_requires_ground_args(eval_table_io) = yes.
>  eval_method_requires_ground_args(eval_memo) = yes.
>  eval_method_requires_ground_args(eval_minimal) = yes.

Hmm, the error message that you will get for violations of this will
be from report_eval_method_requires_ground_args in modes.m, which will
output the following message:

	foo.m:123: Sorry, not implemented: `pragma table_io'
	foo.m:123:   declaration not allowed for procedure with
	foo.m:123:   partially instantiated modes.

This is misleading since there's no `pragma table_io' declaration.

options.m:
> @@ -1678,6 +1684,9 @@
>  %		"\tSuppress the named aspects of the execution tracing system.",
>  		"--trace-optimized",
>  		"\tDo not disable optimizations that can change the trace.",
> +		"--trace-table-io",
> +		"\tEnable the tabling of I/O actions, to allow the debugger",
> +		"\tto execute retry commands across I/O actions.",

The log message said "These options are for developers only for now.",
so that documentation should be commented out.

You should put some documentation for the `--trace-table-io-states'
option in the comments here too.

> +++ compiler/table_gen.m	2000/11/06 03:19:29
> +% Example of transformation for tabling I/O, for I/O primitives that have
> +% tabled_for_io:

It would be helpful to explain here what it means for an I/O primitive to
"have tabled_for_io", and when this will happen.  Also it would help to
explain what you mean by "I/O primitive".

> +%	:- pred p(int, string, io__state, io__state).
> +%	:- mode p(in, out, di, uo) is det.
> +%
> +%	p(A, B, S0, S) :- $pragma(...)
> +%
> +%	The transformed code would be :
> +%
> +%	p(A, B, S0, S) :-
> +%		(if
> +%				% Get the global I/O table, the global I/O
> +%				% counter, and the starting point for tabling
> +%				% I/O actions, if we are in the tabled range.
> +%			table_io_in_range(T0, Counter, Start)
> +%		then
> +%				% Look up the input arguments.
> +%			impure table_lookup_insert_start_int(T0, Counter,
> +%				Start, T),
> +%			(if
> +%				semipure table_io_has_occurred(T)
> +%			then
> +%				impure table_restore_string_ans(T, 0, B)
> +%				impure table_restore_any_ans(T, 1, S)

Is the line immediately above only output if --trace-table-io-states
is enabled?  If so, it would help if you documented that here.

> +%			else
> +%				(
> +%					%
> +%					% Call original procedure
> +%					%
> +%				),
> +%					% Save the answers in the table.
> +%				impure table_io_create_ans_block(T, 2, AB),
> +%				impure table_save_string_ans(AB, 0, B)
> +%				impure table_save_any_ans(AB, 1, S)

Likewise for the line immediately above.

> +%			)
> +%		else
> +%			%
> +%			% Call original procedure
> +%			%
> +%		).
> +%
> +% For I/O primitives that have not_tabled_for_io, we should require that they
> +% do not do any I/O in their own code, meaning that all their I/O is inside
> +% the Mercury code they call. We can then leave such primitives untransformed;
> +% the I/O primitives called from the inner Mercury engine will do the right
> +% thing.

Again, it would be helpful to explain here what it means for an I/O primitive to
"have not_tabled_for_io", and when this can happen.  Is that the default?
If so, lots of stuff will break this requirement...

> @@ -250,27 +311,39 @@
>  	proc_info_goal(ProcInfo0, OrigGoal),
>  	proc_info_argmodes(ProcInfo0, ArgModes),
>  
> +	( EvalMethod = eval_table_io ->
> +		require(unify(CodeModel, model_det),
> +			"tabled io proc not det"),

The compiler front-end doesn't guarantee that invariant, I think.
So with this code the compiler might abort, even on valid input.

> +	list__filter(table_gen__var_is_io_state(Module, VarTypes), InputVars,
> +		IoStateAssignFromVars, _SavedInputVars),
> +	(
> +		TableIoStates = yes,
> +		RestoreAnsGoal = RestoreAnsGoal0
> +	;
> +		TableIoStates = no,
> +		(
> +			IoStateAssignFromVars = [IoStateAssignFromVarPrime],
> +			IoStateAssignToVars = [IoStateAssignToVarPrime]
> +		->
> +			IoStateAssignFromVar = IoStateAssignFromVarPrime,
> +			IoStateAssignToVar = IoStateAssignToVarPrime
> +		;
> +			error("create_new_io_goal: one in / one out violation")
> +		),

The compiler front-end doesn't guarantee that invariant, I think.

> @@ -683,6 +898,23 @@
...
> +:- pred table_gen__type_is_io_state(module_info::in, (type)::in) is semidet.
> +
> +table_gen__type_is_io_state(ModuleInfo, Type) :-
> +	type_to_type_id(Type, TypeCtor, []),
> +	type_util__type_id_module(ModuleInfo, TypeCtor, unqualified("io")),
> +	type_util__type_id_name(ModuleInfo, TypeCtor, "state"),
> +	type_util__type_id_arity(ModuleInfo, TypeCtor, 0).

Use type_util__type_is_io_state.

> Index: library/io.m
> @@ -1365,7 +1365,7 @@
>  
>  :- pragma c_code(io__read_line_as_string_2(File::in, Res :: out,
>  			RetString::out, IO0::di, IO::uo),
> -		[will_not_call_mercury, thread_safe],
> +		[will_not_call_mercury, tabled_for_io, thread_safe],
>  "
>  #define ML_IO_READ_LINE_GROW(n)	((n) * 3 / 2)
>  #define ML_IO_BYTES_TO_WORDS(n)	(((n) + sizeof(MR_Word) - 1) / sizeof(MR_Word))
> @@ -1511,7 +1511,7 @@
>  % same as ANSI C's clearerr().
>  
>  :- pragma c_code(io__clear_err(Stream::in, _IO0::di, _IO::uo),
> -		[will_not_call_mercury, thread_safe],
> +		[will_not_call_mercury, tabled_for_io, thread_safe],
>  "{
>  	MercuryFile *f = (MercuryFile *) Stream;
>  
... and so on for about 700 lines of diff.

The log message says

	library/io.m:
		Mark the relevant procedures with the tabled_for_io feature.

What are the criteria which determine which procedures are "relevant"
here? Are these criteria something that could be automatically applied
by the compiler?  If so, then I think doing it that way would be a
better approach, rather than manually marking them all as
`tabled_for_io'.  If not, then the criteria should be documented.

> Index: library/table_builtin.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/library/table_builtin.m,v
> retrieving revision 1.2
> diff -u -b -r1.2 table_builtin.m
> --- library/table_builtin.m	2000/10/16 01:33:52	1.2
> +++ library/table_builtin.m	2000/11/01 08:16:00
> @@ -316,6 +316,112 @@
>  
>  :- interface.
>  
> +:- import_module io.
> +
> +:- impure pred table_io_in_range(ml_table::out, int::out, int::out) is semidet.
> +
> +:- impure pred table_io_has_occurred(ml_table::in) is semidet.
> +
> +:- pred table_io_copy_io_state(io__state::di, io__state::uo) is det.

You should document what those procedures are supposed to do.

Also please include a comment "N.B. interface continued below"
like in the other interface sections.

> +:- pragma c_code(table_io_copy_io_state(S0::di, S::uo),
> +		[will_not_call_mercury],
> +"
> +	S = S0;
> +").

What's the point of this procedure?
I notice now that table_gen generates calls to it,
but the documentation in table_gen doesn't say anything about it.

> Index: runtime/mercury_wrapper.c
> +
> +/*
> +** These variables are documented in library/table_builtin.m.
> +*/
> +
> +int		MR_io_tabling_phase = 0;
> +bool		MR_io_tabling_enabled = FALSE;
> +MR_TableNode	MR_io_tabling_pointer = { 0 };
> +MR_Unsigned	MR_io_tabling_counter = 0;
> +MR_Unsigned	MR_io_tabling_counter_hwm = 0;
> +MR_Unsigned	MR_io_tabling_start = 0;
> +MR_Unsigned	MR_io_tabling_end = 0;
>  
>  #ifdef USE_GCC_NONLOCAL_GOTOS
>  
> Index: runtime/mercury_wrapper.h
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/mercury_wrapper.h,v
> retrieving revision 1.37
> diff -u -b -r1.37 mercury_wrapper.h
> --- runtime/mercury_wrapper.h	2000/11/05 12:04:19	1.37
> +++ runtime/mercury_wrapper.h	2000/11/06 01:49:57
> @@ -153,6 +153,14 @@
>  */
>  extern	void		(*MR_register_module_layout)(const MR_Module_Layout *);
>  
> +extern	int		MR_io_tabling_phase;
> +extern	bool		MR_io_tabling_enabled;
> +extern	MR_TableNode	MR_io_tabling_pointer;
> +extern	MR_Unsigned	MR_io_tabling_counter;
> +extern	MR_Unsigned	MR_io_tabling_counter_hwm;
> +extern	MR_Unsigned	MR_io_tabling_start;
> +extern	MR_Unsigned	MR_io_tabling_end;

Why are these defined in mercury_wrapper.{c,h}?
Wouldn't mercury_trace_base.{c,h} be a better place?

The pointer to the documentation in library/table_builtin.m
should be put in the header file too.

I think it would be worth including some brief comments
here explaining the meaning of each variable, in
addition to the documentation in library/table_builtin.m.
E.g.

	/*
	** This variable records which phase we're in:
	** Phase 0: program start          to first debugger event.
	** Phase 1: first debugger event   to io_tabling_start event.
	** Phase 2: io_tabling_start event to io_tabling_end event.
	** Phase 3: io_tabling_end event   to end of program.
	** See the comments in library/table_builtin.m for more details.
	*/
	extern	int		MR_io_tabling_phase;

	/* True if I/O tabling has been enabled */
	extern	bool		MR_io_tabling_enabled;

	/* The root of the trie that we use for tabling I/O */
	extern	MR_TableNode	MR_io_tabling_pointer;

	/* The current I/O state number */
	extern	MR_Unsigned	MR_io_tabling_counter;

	/* The highest I/O state number reached ("hwm" = "high water mark") */
	extern	MR_Unsigned	MR_io_tabling_counter_hwm;

	/* The event number at which to start I/O tabling */
	extern	MR_Unsigned	MR_io_tabling_start;

	/* The event number at which to stop I/O tabling */
	extern	MR_Unsigned	MR_io_tabling_end;

It might also help to do

	typedef MR_Unsigned MR_IO_StateNumber;
	typedef MR_Unsigned MR_EventNumber;

and to use those typedefs in appropriate places.
And it might also be worth using a enum for the phase.

> Index: tests/debugger/tabled_read.inp
> ===================================================================
> RCS file: tabled_read.inp
> diff -N tabled_read.inp
> --- /dev/null	Thu Sep  2 15:00:04 1999
> +++ tabled_read.inp	Tue Oct 31 10:10:16 2000
> @@ -0,0 +1,13 @@
> +echo on
> +register --quiet
> +break tabled_read__test
> +table_io start
> +continue
> +finish -n
> +print *
> +table_io
> +retry
> +print *
> +finish -n
> +print *
> +continue -S

Is there currently a way to turn tabled I/O off?
If so, it would be a good idea to test that.

> Index: tests/debugger/tabled_read.m
> ===================================================================
> RCS file: tabled_read.m
> diff -N tabled_read.m
> --- /dev/null	Thu Sep  2 15:00:04 1999
> +++ tabled_read.m	Wed Oct 25 16:49:30 2000
...
> +% We define our own version of io__read_char_code, in case the library
> +% was compiled without IO tabling.
> +
> +:- pred tabled_read__read_char_code(io__input_stream::in, int::out,
> +	io__state::di, io__state::uo) is det.
> +
> +:- pragma c_code(tabled_read__read_char_code(File::in, CharCode::out,
> +		IO0::di, IO::uo), [will_not_call_mercury], "

Don't you need `tabled_for_io' here?
Why/why not?

> +	extern	int	mercury_getc(MercuryFile* mf);
> +	CharCode = mercury_getc((MercuryFile *) File);
> +	IO = IO0;

It would be better to just use getchar() rather than mercury_getc().
That will ensure that this test case is robust against changes
to the Mercury stream implementation.

> +% % We define our own version of io__write_string, in case the library
> +% % was compiled without IO tabling.
> +% 
> +% :- pragma c_code(tabled_read__write_string(Stream::in, Message::in,
> +% 		IO0::di, IO::uo), [may_call_mercury, thread_safe],
> +% "{
> +% 	MercuryFile *stream = (MercuryFile *) Stream;
> +% 	mercury_print_string(stream, Message);
> +% 	update_io(IO0, IO);
> +% }").

The same two points apply here (with s/getchar/printf/
and s/mercury_getc/mercury_print_string).

> Index: trace/mercury_trace.c
...
> +static MR_Unsigned
> +MR_find_saved_io_counter(const MR_Stack_Layout_Label *call_label,
> +	MR_Word *base_sp, MR_Word *base_curfr)
> +{
> +	const MR_Stack_Layout_Entry	*level_layout;
> +	MR_Unsigned			saved_io_counter;
> +
> +	level_layout = call_label->MR_sll_entry;
> +	if (level_layout->MR_sle_maybe_io_seq <= 0) {
> +		MR_fatal_error("MR_trace_retry: "
> +			"missing io seq number slot");
> +	}
> +
> +	if (MR_DETISM_DET_STACK(level_layout->MR_sle_detism)) {
> +		saved_io_counter = MR_based_stackvar(base_sp,
> +			level_layout->MR_sle_maybe_io_seq);
> +	} else {
> +		saved_io_counter = MR_based_framevar(base_curfr,
> +			level_layout->MR_sle_maybe_io_seq);
> +	}

It would be cleaner if some of that was abstracted out into a macro
(or function), i.e.

	#define MR_based_slot(layout, base_sp, base_curfr, slot)	\
		( MR_DETISM_DET_STACK((layout)->MR_sle_detism) ?	\
			MR_based_stackvar((base_sp), (slot))		\
		:							\
			MR_based_framevar((base_curfr), (slot))		\
		)							\

or perhaps

	#define MR_based_slot(detism, base_sp, base_curfr, slot)	\
		( MR_DETISM_DET_STACK(detism) ?				\
			MR_based_stackvar((base_sp), (slot))		\
		:							\
			MR_based_framevar((base_curfr), (slot))		\
		)							\

This is also used once in runtime/mercury_stack_trace.c.

> +++ trace/mercury_trace_internal.c	2000/11/06 03:47:17
> @@ -2003,6 +2077,23 @@
...
> +static void
> +MR_print_unsigned_var(FILE *fp, const char *var, MR_Unsigned value)
> +{
> +	fprintf(fp, "%s = ", var);
> +	MR_print_unsigned(fp, value);
> +	fprintf(fp, "\n");
> +}
> +
> +static void
> +MR_print_unsigned(FILE *fp, MR_Unsigned value)
> +{
> +	char	buf[20];
> +
> +	sprintf(buf, "%%%su", MR_INTEGER_LENGTH_MODIFIER);
> +	fprintf(fp, buf, value);
> +}

A simpler method is to use ANSI/ISO C string concatenation:

	static void
	MR_print_unsigned_var(FILE *fp, const char *var, MR_Unsigned value)
	{
		fprintf(fp, "%s = %" MR_INTEGER_LENGTH_MODIFIER "u\n",
			var, value);
	}

----------

That's it for this review.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
                                    |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------