[m-dev.] for review: independent AND parallelism [runtime]

[Fergus Henderson]

Hi Tom,

Thanks for those changes.

On 27-Apr-1998, Thomas Charles CONWAY <conway at cs.mu.OZ.AU> wrote:
> compiler/instmap.m:
> 	add instmap__unify which takes a list of instmaps
> 		and abstractly unifies them.
> 	add unify_instmap_delta which tajes two instmap deltas
> 		and abstractly unifies them.

s/tajes/takes/

> library/Makefile:
> 	work around a bug in the solaris version pthread.h

s/version/version of/ ?

> library/{nc,sp}_builtin.nl:
> 	add an op declaration for &.

Shouldn't you also modify library/ops.m and
compiler/mercury_to_mercury.m?

> diff -u -r1.1 mercury_context.c
...
> +#ifdef MR_THREAD_SAFE
> +#include "mercury_thread.h"
>  #endif

Please indent the #include by two spaces,
since it's inside an #ifdef.

> +#ifdef	MR_THREAD_SAFE
> +MercuryLock	*MR_runqueue_lock;
> +MercuryCond	*MR_runqueue_cond;
> +#endif

Ditto here.  Probably also in lots of other places
in runtime/*.[ch].

> +#define runnext()	do {					\
> +		Context *rn_c, *rn_p;				\
> +		unsigned x;					\
> +		MercuryThread t;				\
> +		x = MR_ENGINE(c_depth);				\
> +		t = MR_ENGINE(owner_thread);			\
> +		MR_LOCK(MR_runqueue_lock, "runnext i");		\
> +		while (1) {					\
> +			if (MR_exit_now == TRUE)		\
> +				destroy_thread(MR_engine_base);	\
> +			rn_c = MR_runqueue;			\
> +			rn_p = NULL;				\
> +			while (rn_c != NULL) {			\
> +				if ( (x > 0 && rn_c->owner_thread == t) \
> +				   ||	(rn_c->owner_thread == NULL)) \
> +					break;			\
> +				rn_p = rn_c;			\
> +				rn_c = rn_c->next;		\
> +			}					\
> +			if (rn_c != NULL)			\
> +				break;				\
> +			MR_WAIT(MR_runqueue_cond, MR_runqueue_lock); \
> +		}						\
> +		MR_ENGINE(this_context) = rn_c;			\
> +		if (rn_p == NULL)				\
> +			MR_runqueue = rn_c->next;		\
> +		else						\
> +			rn_p->next = rn_c->next;		\
> +		MR_UNLOCK(MR_runqueue_lock, "runnext");		\
> +		load_context(MR_ENGINE(this_context));		\
> +		GOTO(MR_ENGINE(this_context)->resume);		\
> +	} while(0)

That macro is pretty complicated.  I think that most of that stuff
probably should go in a function, rather than a macro.

> +#ifdef	MR_THREAD_SAFE
> +#define	IF_MR_THREAD_SAFE(x)	x
> +#else
> +#define IF_MR_THREAD_SAFE(x)
> +#endif

Hmm, that's the second occurrence of that...

> +#define fork_new_context(child, parent, numslots) do {			\
> +		Context	*f_n_c_context;					\
> +		int	fork_new_context_i;				\
> +		MR_LOCK(MR_runqueue_lock, "fork");			\
> +		MR_UNLOCK(MR_runqueue_lock, "fork");			\
> +		f_n_c_context = create_context();			\
> +		IF_MR_THREAD_SAFE(					\
> +			f_n_c_context->owner_thread = NULL;		\
> +		)							\
> +		for (fork_new_context_i = (numslots) ;			\
> +				fork_new_context_i > 0 ;		\
> +				fork_new_context_i--) {			\
> +			*(f_n_c_context->context_sp) = 			\
> +				detstackvar(fork_new_context_i);	\
> +			f_n_c_context->context_sp++;			\
> +		}							\
> +		f_n_c_context->resume = (child);			\
> +		schedule(f_n_c_context);				\
> +		GOTO((parent));						\
> +		}							\

This is another complicated macro; does all of this
really need to be in a macro rather than a function?

I don't understand the call to MR_LOCK immediately
followed by a corresponding call to MR_UNLOCK.

> +#ifdef	MR_THREAD_SAFE
> +#define MR_init_sync_term(sync_term, nbranches)	do {			\
> +		SyncTerm *st = (SyncTerm *) sync_term;			\
> +		pthread_mutex_init(&(st->lock), MR_MUTEX_ATTR);		\
> +		st->count = (nbranches);				\
> +		st->parent = NULL;					\
> +	} while (0)
> +#else
> +#define MR_init_sync_term(sync_term, nbranches)	do {			\
> +		SyncTerm *st = (SyncTerm *) sync_term;			\
> +		st->count = (nbranches);				\
> +		st->parent = NULL;					\
> +	} while (0)
> +#endif

You could use MR_IF_THREAD_SAFE() to avoid the code dupliation here.

> +++ mercury_engine.c	1998/03/11 04:52:30
>  void 
> +init_engine(MercuryEngine *eng)
>  {
> -	init_memory();
> +	/* XXX */ init_memory();

You should explain the XXX.

> +void finalize_engine(MercuryEngine *eng)
> +{
> +	
> +}

Hmm, maybe this needs an XXX comment?

> Index: runtime/mercury_engine.h
>    /*
>    ** When restoring hp, we must make sure that we don't truncate the heap
>    ** further than it is safe to. We can only truncate it as far as
> +  ** min_heap_reclamation_point. See the comments in mercury_context.h next
> +  ** to the set_min_heap_reclamation_point() macro.
>    */
> +  #define	restore_hp(src)	(					\
> +  			LVALUE_CAST(Word,MR_hp) = (src),		\
> +  			(void)0						\
> +  		)
>  
> +  /*
> +  #define	restore_hp(src)	(					\
> +  			LVALUE_CAST(Word,MR_hp) =			\
> +  			  ( (Word) MR_min_hp_rec < (src) ?		\
> +  			  (src) : (Word) MR_min_hp_rec ),		\
> +  			(void)0						\
> +  		)
> +  */

You need to explain why the second version is commented out.

> Index: runtime/mercury_imp.h
>  void 
>  init_memory(void)
>  {
> +	static first_time_only=0;
>
> +	if (first_time_only != 0)
> +		return;
> +	first_time_only = 1;

You should avoid implicit int declarations --
as well as being bad style, they're banned by C9X.
And some whitespace around the `=' would be a good idea.

But I think it would be better to use `bool' instead of `int'
and `TRUE' and `FALSE' instead of `0' and `1'
(perhaps changing the name from `first_time_only' to
`already_done').

> Index: runtime/mercury_memory.h
...
> +/*
> +** destroy_zone(Zone) destroys a memory zone.
> +*/

How about

> --- mercury_misc.c	1998/01/23 15:26:33	1.3
> +++ mercury_misc.c	1998/03/18 23:48:42
> +void
> +MR_copy_structure(char *dest, const char *src, size_t nbytes)
> +{
> +	while (nbytes-- > 0)
> +		*dest++ = *src++;
> +}

I think `MR_memcpy()' would be a better name.
There should be a comment explaining why you don't just use memcpy().
Also please use {}.

> Index: runtime/mercury_wrapper.c
...
>  	/* start up the Mercury engine */
> -	init_engine();
> +#ifndef MR_THREAD_SAFE
> +	init_thread((void *) 1);

What is this `(void *)1' doing here?
What's the magic number `1' indicate?

> #ifndef MERCURY_THREAD_H
> #define MERCURY_THREAD_H
> 
> #ifdef	MR_THREAD_SAFE
> 
> #include <signal.h>	/* for sigset_t on the SPARC */
> #include <pthread.h>
> #include "mercury_std.h"

"

> #if defined(__alpha__)
> #define MR_MUTEX_ATTR	pthread_mutexattr_default
> #define MR_COND_ATTR	pthread_condattr_default
> #define MR_THREAD_ATTR	pthread_attr_default
> #else
> #define MR_MUTEX_ATTR	NULL
> #define MR_COND_ATTR	NULL
> #define MR_THREAD_ATTR	NULL
> #endif

`__alpha__' is not the right test -- no doubt this will break on Linux/Alpha.
Ideally of course it should be autoconf'ed.  
But at very least you should add a level of indirection:

	/* this code should go in mercury_conf_param.h.in */
	#if defined(__alpha__) && defined(OSF or something like that)
	  #define MR_USE_ALPHA_STYLE_PTHREADS
	#endif

	/* in mercury_thread.h */
	#ifdef MR_USE_ALPHA_STYLE_PTHREADS
	  #define MR_MUTEX_ATTR		pthread_mutexattr_default
	  #define MR_COND_ATTR		pthread_condattr_default
	  #define MR_THREAD_ATTR	pthread_attr_default
	#else
	  #define MR_MUTEX_ATTR		NULL
	  #define MR_COND_ATTR		NULL
	  #define MR_THREAD_ATTR	NULL
	#endif

> #if 0
> #define	MR_LOCK(lck, from)	pthread_mutex_lock((lck))
> #define	MR_UNLOCK(lck, from)	pthread_mutex_unlock((lck))
> 
> #define	MR_SIGNAL(cnd)		pthread_cond_signal((cnd))
> #define	MR_WAIT(cnd, mtx)	pthread_cond_wait((cnd), (mtx))
> #else
> void MR_mutex_lock(MercuryLock *lock, const char *from);
> void MR_mutex_unlock(MercuryLock *lock, const char *from);
> void MR_cond_signal(MercuryCond *cond);
> void MR_cond_wait(MercuryCond *cond, MercuryLock *lock);
> 
> #define	MR_LOCK(lck, from)	MR_mutex_lock((lck), (from))
> #define	MR_UNLOCK(lck, from)	MR_mutex_unlock((lck), (from))
> 
> #define	MR_SIGNAL(cnd)		MR_cond_signal((cnd))
> #define	MR_WAIT(cnd, mtx)	MR_cond_wait((cnd), (mtx))
> #endif

Please explain the `#if 0'.

> #if defined(__alpha__)
> #define MR_GETSPECIFIC(key) 	({			\
> 		pthread_addr_t gstmp;			\
> 		pthread_getspecific((key), &gstmp);	\
> 		(void *) gstmp;				\
> 	})
> #define	MR_KEY_CREATE	pthread_keycreate
> #else
> #define MR_GETSPECIFIC(key)	pthread_getspecific((key))
> #define	MR_KEY_CREATE		pthread_key_create
> #endif

Again you shouldn't test `__alpha__' here.

> bool	MR_exit_now;

Shouldn't that be `volatile'?

> BEGIN_MODULE(scheduler_module)
> 	init_entry(do_runnext);
> BEGIN_CODE
> 
> Define_entry(do_runnext);
> 	runnext();
> 
> 	fatal_error("Execution should never reach here.");
> 
> END_MODULE
> 
> void mercury_scheduler_wrapper(void); /* suppress gcc warning */
> void mercury_scheduler_wrapper(void) {
> 	scheduler_module();
> }

I think you need a

	/*
	** INIT mercury_scheduler_wrapper
	*/

comment somewhere.

Apart from that, the changes in this email look fine.

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