[m-dev.] changes to runtime

[Fergus Henderson]

Thomas Charles CONWAY, you wrote:
> +/*
> +** If we have parallelism enabled, we need a global semaphore for
> +** the runqueue.
> +*/
> +#ifdef	MR_PARALLEL
> +extern sem_t	mr_runqueue_sem;
> +#endif

Please use `MR_', not `mr_'.
(Ditto in lots of places.)

> +#ifdef	MR_PARALLEL
> +pthread_d	my_thread_id;
> +int		my_thread_num;
> +Word		*min_heap_reclamation_point;
> +#endif

Don't they need to go in the per-thread struct?

> -	MR_assert(free_context_list_ptr != NULL);
> -	if (*free_context_list_ptr == NULL) {
> -		fatal_error("no free contexts");
> +	MR_assert(free_context_list != NULL);
> +	if (free_context_list == NULL) {
> +		c = allocate_object(Context);

I think the call to MR_assert() is wrong.

> -	MR_assert(free_context_list_ptr != NULL);
> -	c->next = *free_context_list_ptr;
> -	*free_context_list_ptr = c;
> +	MR_assert(free_context_list != NULL);
> +	c->next = free_context_list;
> +	free_context_list = c;

Ditto.

> +#ifdef	MR_PARALLEL
> +#include <pthread.h>		/* for the various pthread bits */
> +#endif

The comment is not the most informative I've seen... ;-)

> +#ifndef	MR_PARALLEL
> +
> +#ifdef	MR_USE_TRAIL
> +#define	load_trail(engine_ptr)		do {				\

The conditional compilation is quite difficult to follow here.
Please indent everything that is conditionally compiled by
two spaces per level of #if.
Please add comments to every #else and #endif in this part.

> +#define	load_engine(engine_ptr)		\
> +		pthread_setspecific(mr_engine_base_key, (void *) (engine_ptr))

That should be `(void **) &engine_ptr', I think.

> +#define save_engine(engine_ptr)		\
> +		pthread_getspecific(mr_engine_base_key, (void *) (engine_ptr))

The cast to `void *' here should be unnecessary.

> +extern	MercuryEngine	*create_engine(void);
> +
> +extern	void		init_engine(void);
> +extern	void		call_engine(Code *entry_point);
> +extern	void		terminate_engine(void);
> +extern	void		dump_prev_locations(void);

Hmmm...  shouldn't these functions have MercuryEngine parameters?

>  void 
>  init_engine(void)
>  {
> +	MercuryEngine	*e;
> +#ifdef	MR_PARALLEL
> +	int	err;
> +#endif

That variable appears to be unused.

> +static void
> +init_threads(void)
> +{
> +	int n, err;
> +
> +	for (n = 1; n < numprocs; n++)
> +	{

Put the curly on the same line as the for().

> +		mr_engine_bases[n] = create_engine();
> +		err = pthread_create(mr_threads+n, MR_THREAD_ATTRS,

s/+/ + /

or s/mr_threads+n/&mr_threads[n]/  -- that might be cleaner.

> +			start_thread, (void *) mr_engine_bases[n]);
> +		if (err != 0)
> +		{

Another misplaced curly.

> +			fatal_error("unable to create thread.\n");

Delete the ".\n"

> +MR_THREAD_RETURN_T start_thread(void *v)
> +{
> +	MercuryEngine *e;
> +
> +	e = (MercuryEngine *) v;
> +	pthread_setspecific(mr_engine_base_key, e);
> +	call_engine(ENTRY(do_runnext));
> +	/* not reached */
> +	MR_assert(FALSE);
> +}

Some comments please.

> +MercuryEngine *create_engine(void)
> +{
> +	MercuryEngine *e;

Please s/e/engine/

> +	e = allocate_object(MercuryEngine);
> +	if (!e)
> +		fatal_error("failed to allocate MercuryEngine.\n");

Use curlies please, or put the fatal_error() on the same line as the `if'.
Delete the ".\n".

> +#ifdef MR_USE_TRAIL
> +	e->e_tail_zone = NULL;

s/tail/trail/

> -Word	fake_reg[MAX_FAKE_REG];
> +#ifndef MR_PARALLEL
> +  Word	*fake_reg;
> +#endif

Please don't do that; it will make the code much less efficient.

> +#ifdef MR_USING_TRAIL

s/USING/USE/

>  /* reserve MAX_FAKE_REG virtual regs, numbered from 0 to MAX_FAKE_REG-1 */
> -extern	Word	fake_reg[MAX_FAKE_REG];
> +#ifndef	MR_PARALLEL
> +extern	Word	*fake_reg;

Please don't do that; it will make the code much less efficient.

> +#ifdef MR_USING_TRAIL

s/USING/USE/

> +#else
> +
> +#define detstack_zone		(mr_engine_base->e_detstack_zone)

Please change
	#else
to
	#else /* MR_PARALLEL */

>  #ifndef REGORDER_H
>  #define REGORDER_H
>  
> +#if	!defined(MR_PARALLEL) || (NUM_REAL_REGS < 1)
> +

Please add a comment explaining this #if.

> +#ifndef	MR_PARALLEL
>  #define save_registers() 	save_regs_to_mem(fake_reg)
> +#else
> +#error need a definition of save_registers
> +#endif
> +
> +/* 
> +** the save_arg_registers() macro copies the physical machine registers
> +** that are being used for storing the rN and fN registers to their
> +** corresponding slots in the fake_reg array 
> +** XXX
> +*/
> +
> +#define save_arg_registers() 	save_regs_to_mem(fake_reg)

Explain the XXX.

> +/* 
> +** the restore_arg_registers() macro sets the physical machine registers
> +** that are being used for storing the rN and fN registers to the values
> +** in their corresponding slots in the fake_reg array 
> +** XXX
> +*/
> +
> +#define restore_arg_registers() 	restore_regs_from_mem(fake_reg)

Ditto.

> +#ifdef	MR_PARALLEL
> +
> +MR_Sem *allocate_semaphore(void)
> +{
> +	MR_Sem *s;
> +	int err;
> +
> +	s = allocate_object(MR_Sem);
> +	err = sem_init(s, 0, 0);

Please document the meaning of the two 0s.
In fact (he says after checking the man page)
also change the first one from `0' to `FALSE'.

> +	if (err != 0)
> +		fatal_error("failed to initialize semaphore");

Please add curlies.

> +	if (err != 0)
> +		fatal_error("attempt to deallocate semaphore with blocked threads");

Ditto.

>  	/* double-check that the garbage collector knows about
>  	   global variables in shared libraries */
> +/* XXX moved to engine.mod
>  	GC_is_visible(fake_reg);
> +*/

Can you please find some other global variable and use that,
rather than commenting the test out here?

When you've addressed the above issues, please send another diff.

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