[m-rev.] for review: debugging tabling procedures

[Fergus Henderson]

On 08-Nov-2002, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Index: compiler/hlds_pred.m
> +:- type table_trie_step
> +	--->	table_trie_step_int
> +	;	table_trie_step_char
> +	;	table_trie_step_string
> +	;	table_trie_step_float
> +	;	table_trie_step_enum(int)
> +	;	table_trie_step_user(type)
> +	;	table_trie_step_poly.

The meaning of the int in step_enum(int) should be documented.

> +		% The information we need to interpret the data structures
> +		% created by tabling for a procedure, except the information
> +		% (such as determinism) that is already available from
> +		% proc_layout structures.
> +		%
> +		% The table_arg_type_infos list first all the input arguments,
> +		% then all the output arguments.
> +	;	table_gen_info(
> +			num_inputs ::		int,
> +			num_outputs ::		int,
> +			input_steps ::		list(table_trie_step),
> +			gen_arg_infos ::	table_arg_infos
>  		).

What happens with arguments whose arg_mode is top_unused?

> + 			maybe_table_info :: maybe(proc_table_info),
> +					% If set, it means that procedure
> +					% has been subject to a tabling
> +					% transformation, either I/O tabling
> +					% or the regular kind. In the former
> +					% case, the argument will contain all
> +					% the information we need to display
> +					% I/O actions involving this procedure;
> +					% in the latter case, it will contain
> +					% all the information we need to display
> +					% the call tables, answer tables and
> +					% answer blocks of the procedure.
> +					% XXX For now, the compiler fully
> +					% supports only procedures whose
> +					% arguments are all builtin types,
> +					% and the debugger further restricts
> +					% this to procedures whose arguments
> +					% are integers. However, this is still
> +					% sufficient for debugging most
> +					% problems in the tabling system.

Is the part "the debugger further restricts this to procedures whose
arguments are integers" still correct?

> Index: compiler/table_gen.m
...
> @@ -1824,17 +1856,35 @@
>  not_builtin_type(tuple_type).
>  not_builtin_type(user_type).
>  
> +:- pred builtin_type_lookup_category(builtin_type::in, string::out,
> +	table_trie_step::out) is det.
>  
> +builtin_type_lookup_category(int_type, 	 "int",    table_trie_step_int).
> +builtin_type_lookup_category(char_type,  "char",   table_trie_step_char).
> +builtin_type_lookup_category(str_type, 	 "string", table_trie_step_string).
> +builtin_type_lookup_category(float_type, "float",  table_trie_step_float).
> +builtin_type_lookup_category(enum_type,  _, _) :-
> +	error("builtin_type_lookup_category: non-builtin-type").
> +builtin_type_lookup_category(pred_type, _, _) :-
> +	error("builtin_type_lookup_category: non-builtin-type").
> +builtin_type_lookup_category(tuple_type,_, _) :-
> +	error("builtin_type_lookup_category: non-builtin-type").
> +builtin_type_lookup_category(polymorphic_type, _, _) :-
> +	error("builtin_type_lookup_category: non-builtin-type").
> +builtin_type_lookup_category(user_type, _, _) :-
> +	error("builtin_type_lookup_category: non-builtin-type").
> +
> +:- pred type_save_category(builtin_type::in, string::out) is det.
> +
> +type_save_category(enum_type, 	 "enum").
> +type_save_category(int_type, 	 "int").
> +type_save_category(char_type, 	 "char").
> +type_save_category(str_type, 	 "string").
> +type_save_category(float_type, 	 "float").
> +type_save_category(pred_type, 	 "pred").
> +type_save_category(tuple_type,	 "any").
> +type_save_category(polymorphic_type,  "any").
> +type_save_category(user_type, 	 "any").

Some comments here explaining the meaning of these routines would be
very helpful.

> Index: library/string.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/library/string.m,v
> retrieving revision 1.183
> diff -u -b -r1.183 string.m
> --- library/string.m	29 Oct 2002 07:11:12 -0000	1.183
> +++ library/string.m	6 Nov 2002 09:03:03 -0000
> @@ -1877,6 +1877,9 @@
>  	** double or float.  The %c checks for any erroneous characters
>  	** appearing after the float; if there are then sscanf() will
>  	** return 2 rather than 1.
> +	**
> +	** The logic used here is duplicated in the function MR_trace_is_float
> +	** trace/mercury_trace_util.c.

	** in trace/mercury_trace_util.c.
	   ^^

> Index: runtime/mercury_stack_layout.h
> @@ -453,6 +449,64 @@
>  } MR_Table_Io_Decl;
>  
>  /*
> +** The MR_Table_Gen structure.
> +** Each argument of a tabled predicate is an input or an output.

What about arguments with mode `unused'?
What happens for those?

> Index: runtime/mercury_tabling.c
...
> @@ -373,14 +387,20 @@
>                  slot = slot->next;                                            \
>          }                                                                     \
>                                                                                \
> +    /* Check whether we are allowed to add the element. */                  \
> +    if (insert_only) {                                                      \
> +        return NULL;                                                        \
> +    }                                                                       \

Shouldn't that be spelt "lookup_only", rather than "insert_only"? 
With the current spelling, it seems to do the opposite of what its name
suggests.

> +#define MR_HASH_CONTENTS_FUNC_BODY                                      \
> +        MR_bool                                                         \
> +        func_name(MR_TrieNode t, type_name **values_ptr,                \
> +            int *value_next_ptr)                                        \
> +        {                                                               \

A comment here explaining what this macro does (and/or what the
functions that it generates do) would be helpful.

> Index: trace/mercury_trace_internal.c
...
> +static void
> +MR_print_answerblock(const MR_Proc_Layout *proc, MR_Word *answer_block)
> +{
...
> +#ifdef	MR_HIGHLEVEL_CODE
> +				(double) MR_unbox_float(answer_block[i]));
> +#else
> +				(double) MR_word_to_float(answer_block[i]));
> +#endif

The argument to MR_unbox_float() should have type `MR_Box', not
`MR_Word', so you should use a cast to MR_Box there.

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