[m-dev.] for review: cleanup of tabling

[Fergus Henderson]

On 30-Dec-1999, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> Index: runtime/mercury_stacks.c
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/mercury_stacks.c,v
> retrieving revision 1.4
> diff -u -b -r1.4 mercury_stacks.c
> --- runtime/mercury_stacks.c	1999/10/18 15:47:00	1.4
> +++ runtime/mercury_stacks.c	1999/12/30 08:21:30
> @@ -10,10 +10,10 @@
>  #ifdef	MR_USE_MINIMAL_MODEL
>  
>  static	void	MR_print_gen_stack_entry(FILE *fp, Integer i);
> -static	void	MR_cleanup_generator_ptr(MR_Subgoal **generator_ptr);
> +static	void	MR_cleanup_generator_ptr(MR_TrieNode generator_ptr);

Hmm, does that function assume that the generator_ptr
uses a particular member of the union?
I think it does.  It would be good to at least document that.

>  void
> -MR_push_generator(Word *frame_addr, MR_Subgoal *table_addr)
> +MR_push_generator(Word *frame_addr, MR_TrieNode table_addr)

Likewise here.

>  void
> -MR_register_generator_ptr(MR_Subgoal **generator_ptr)
> +MR_register_generator_ptr(MR_TrieNode generator_ptr)

And here.

>  static void
> -MR_cleanup_generator_ptr(MR_Subgoal **generator_ptr)
> +MR_cleanup_generator_ptr(MR_TrieNode generator_ptr)
>  {

And here.

> +++ runtime/mercury_stacks.h	1999/12/30 08:21:30
> @@ -357,11 +357,11 @@
>  
>  typedef struct MR_GeneratorStackFrameStruct {
>  	Word			*generator_frame;
> -	MR_Subgoal		*generator_table;
> +	MR_TrieNode		generator_table;
>  } MR_GeneratorStackFrame;
>  
>  extern	void			MR_push_generator(Word *frame_addr,
> -					MR_Subgoal *table_addr);
> +					MR_TrieNode table_addr);
>  extern	MR_Subgoal		*MR_top_generator_table(void);
>  extern	void			MR_pop_generator(void);
>  extern	void			MR_print_gen_stack(FILE *fp);
> @@ -370,7 +370,7 @@
>  
>  typedef struct MR_CutGeneratorListNode *MR_CutGeneratorList;
>  struct MR_CutGeneratorListNode {
> -	MR_Subgoal		**generator_ptr;
> +	MR_TrieNode		generator_ptr;
>  	MR_CutGeneratorList	next_generator;
>  };
>  
> @@ -383,7 +383,7 @@
>  extern	void			MR_commit_mark(void);
>  extern	void			MR_commit_cut(void);
>  
> -extern	void			MR_register_generator_ptr(MR_Subgoal **);
> +extern	void			MR_register_generator_ptr(MR_TrieNode);

And likewise for all of these changes.

> --- runtime/mercury_tabling.c	1999/12/11 15:32:32	1.17
> +++ runtime/mercury_tabling.c	1999/12/30 08:39:30
...
> +/*
> +** The elements field points to an array of size slots, of which user_slots
> +** are occupied. When the ratio of occupied to total slots exceeds
> +** MAX_EL_SIZE_RATIO, we increase the table size to the next prime.
> +*/
>  

s/user_slots/used_slots/

Also I think it would be clearer if the field names were in quotes
in the comment: `elements', `size', `user_slots'.

>  /*
> +** Prime numbers which are close to powers of 2.  Used for choosing
> +** the next size for a hash table.
> +*/

That comment seems to have been moved to after the table of
primes to which it originally applied.

> +#define	MR_generic_lookup_or_add					      \
> +	MR_HashTable	*table;						      \
> +	Integer		bucket;						      \
...

That macro is not a function-like macro, so it
should have its name in upper case.

> +MR_TrieNode
> +MR_type_info_lookup_or_add(MR_TrieNode table, Word *type_info)
> +{
> +	MR_TypeInfo	collapsed_type_info;
> +	MR_TypeCtorInfo	type_ctor_info;
> +	MR_TrieNode	node;
> +	Word		**type_info_args;
> +	int		i;
> +
> +	/* XXX memory allocation here should be optimized */
> +	collapsed_type_info = MR_collapse_equivalences((Word) type_info);
> +
> +	type_ctor_info = MR_TYPEINFO_GET_TYPE_CTOR_INFO(type_info);

Why do you use `type_info' here, rather than `collapsed_type_info'?

> +	node = MR_int_hash_lookup_or_add(table, (Integer) type_ctor_info);
> +
> +	/*
> +	** All calls to MR_type_info_lookup_or_add that have the same value
> +	** of node at this point agree on the type_ctor_info of the type
> +	** being tabled. They must therefore also agree on its arity.
> +	** This is why looping over all the arguments works.
> +	**
> +	** XXX maybe not for ho types
> +	*/
> +
> +	type_info_args = (Word **) collapsed_type_info;
> +
> +	for (i = 1; i <= type_ctor_info->arity; i++) {
>  	}

That loop looks incomplete -- why is the loop body empty?

> --- /dev/null	Thu Sep  2 15:00:04 1999
> +++ mercury_tabling_macros.h	Thu Dec 30 20:23:31 1999
> +#ifdef CONSERVATIVE_GC
> +
> +  #define MR_TABLE_SAVE_ANSWER(table, offset, value, type_info)		\
> +	do {								\
> +		(table)->MR_answerblock[offset] = value;		\
> +	} while(0)
> +
> +#else /* not CONSERVATIVE_GC */
> +
> +  #define MR_TABLE_SAVE_ANSWER(table, offset, value, type_info)		\
> +	do {								\
> +		save_transient_hp();					\
> +		{							\
> +			Word	local_val = (value);			\
> +			Word	local_type_info = (type_info);		\
> +			(table)->MR_answerblock[(offset)] =		\
> +				deep_copy(&local_val, &tlocal_type_info,\
> +					NULL, NULL);			\
> +		}							\
> +		restore_transient_hp();					\
> +	} while(0)
> +
> +#endif /* CONSERVATIVE_GC */

Now that I think about it a bit more, I see that the code
for the non-conservative-gc case would not work; it makes
a copy, but the copy is allocated on the ordinary heap,
not on the global heap, so it doesn't buy you anything.

That whole `#ifdef ... #endif' should be replaced by code
using a call to MR_make_permanent():

  #define MR_TABLE_SAVE_ANSWER(table, offset, value, type_info)		\
	do {								\
		(table)->MR_answerblock[offset] = 			\
			MR_make_permanent((value), (type_info));	\
	} while(0)

Anyway, I've reviewed all of it now.

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