[m-dev.] response to fjh's review of the parallelism changes

[Fergus Henderson]

On 01-Jun-1998, Thomas Charles CONWAY <conway at cs.mu.OZ.AU> wrote:
> Fergus Henderson, you write:
> > > +/*
> > > +** We use our own version of memcpy because gcc recognises calls to the
> > > +** standard memcpy and generates inline code for them. Unfortunately this
> > > +** causes it to abort because it tries to use a register that we're already
> > > +** reserved.
> > > +*/
> > > +void MR_memcpy(char *dest, const char *src, size_t nbytes);
> > 
> > Which version of gcc, on which architecture?
> > Doesn't compiling with -fno-builtin avoid this?
> > Don't we compile with -fno-builtin by default?
> 
> Sparc (toaster et al), gcc 2.7.2
> 
> -fno-builtin doesn't occur in mgnuc (nor anywhere else in
> the runtime or scripts. It does occur in the configure script though.

Ah yes.  Currently we use -fno-builtin only for x86.

> > > -	MR_engine_base = *eng;
> > > +	MR_memcpy(&MR_engine_base, eng, sizeof(MercuryEngine));
> > >  	restore_registers();
> > 
> > Ah, I see.  This is where MR_memcpy() is used.
> > Did you try using memcpy() here?
> 
> No - when we discovered this was causing the gcc abort, you
> thought (at the time) that memcpy wouldn't help (unless we
> included -fno-builtin, I suppose - perhaps we should).

Yes.  If memcpy() doesn't work, then that is a problem -- for
example, users may include calls to memcpy() in `pragma c_code'
declarations.

However, this should not be a high priority -- don't worry
about it for this change, but please just include your
answers here in the documentation.

mercury/runtime/mercury_conf.h.in:
>  /*
>  ** Multithreaded execution support.
>  */
>  #undef MR_DIGITAL_UNIX_PTHREADS

The comment here doesn't quite match the #undef.

Various places, e.g. mercury_spinlock.[ch], still use #ifdef PARALLEL.
Some other places use MR_THREAD_SAFE.

All these macros should be documented in either mercury_conf.h.in or
mercury_conf_param.h.  Currently neither PARALLEL nor MR_THREAD_SAFE
is documented there.

The thread_safe variable should be set in init_grade_options.sh-subr
only -- there's no need to also set it in mgnuc.in and ml.in,
since those file #include init_grade_options.sh-subr.

There should be an option for parallel or thread safe operation, not just
a grade modifier (`.par'), because every grade modifier should have a
corresponding option.  Oh, I see you already have a `--parallel option'
supported by compiler/options.m.  But parse_grade_options.sh-subr should
support this option too.

> +++ mercury/runtime/mercury_engine.c	Mon Jun  1 11:21:06 1998
> @@ -94,9 +94,14 @@
> +/*
> +** finalize_engine should release any resources used withing the
> +** engine structure (but not the engine structure itself).
> +** Currently, it doesn't need to realase any.
> +*/

s/withing/within/
s/realase/release/

> +/*
> +** The following two macros are used as the argument to
> +** init_thread.
> +** MR_USE_NOW should be passed to init_thread to indicate that
> +** it has been called in a context in which it should initialize
> +** the current thread's environment and return.
> +** MR_USE_LATER should be passed to indicate that the thread should
> +** be initialized, then suspend waiting for work to appear in the
> +** runqueue.
> +*/
> +#define MR_USE_NOW	((void *) 1)
> +#define MR_USE_LATER	((void *) 0)

Ah, you still didn't document why the (void *) casts are needed,
but I understand it now.  They're needed because the
pthreads interface uses `void *' as generic type -- specifically,
pthread_init() takes as arguments a `void * f(void *)' function
and a `void *' data to pass to that function.

It would be cleaner to use an enum instead of #defines, and to put the
casts to void * at the call to pthread_init() and the cast from void *
at the top of the init_thread() function whose address you pass to
pthread_create().  In fact, you could define the interface of
init_thread() in a clean manner

	typedef enum { MR_use_now, MR_use_later } MR_when_to_use;

	void init_thread(MR_when_to_use);

and then use casts and a seperate function with the interface that
pthread_create() wants only at the place where pthread_create()
is actually called:

	static void *
	call_init_thread(void *arg)
	{
		init_thread((MR_when_to_use) arg);
		return NULL;
	}

	MercuryThread *
	create_thread(MR_when_to_use when)
	{
		...
		err = pthread_create(thread, &attrs, call_init_thread,
			(void *) when); 
		...
	}
		
> diff -u -r bak/mercury/runtime/mercury_wrapper.c mercury/runtime/mercury_wrapper.c
> --- bak/mercury/runtime/mercury_wrapper.c	Fri May 22 10:01:32 1998
> +++ mercury/runtime/mercury_wrapper.c	Mon Jun  1 11:35:16 1998
> @@ -225,15 +225,15 @@
>  
>  	/* start up the Mercury engine */
>  #ifndef MR_THREAD_SAFE
> -	init_thread((void *) 1);
> +	init_thread((void *) MR_USE_NOW);
>  #else
>  	{
>  		int i;
>  		init_thread_stuff();
> -		init_thread((void *)1);
> +		init_thread((void *) MR_USE_NOW);

It seems like overkill to cast to `void *' twice,
once in the definition of MR_USE_NOW and once when you use it.

In fact for calls such as these which do not go via pthread_create(),
you can avoid the need for any cast at all, as I explained above.

--------------------

I think we still need another round of reviewing.
Can you please send another relative diff next time?

Thanks,
	Fergus.

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