[m-dev.] diff: Runtime support for stack_layouts

[Zoltan Somogyi]

> runtime/calls.h:
> 	Instead of doing ASM_FIXUP_REGS as part of the call macro, do
> 	it as part of the proceed macro.
> 	As part of the call macro, the succip would always be set
> 	to the "fixup_gp" label, so it was difficult to find the
> 	caller using the succip. Using this technique, it is set
> 	correctly.
> 	(There's a small cost - with the call macro, you only did
> 	ASM_FIXUP_REGS after you returned from a non-local call, using
> 	"proceed" you do this at every call. The only way I can see
> 	to avoid this cost is to actually change the code generator
> 	so that it outputs two labels for continuations, one used
> 	for local calls, and one for non-local calls. The nonlocal
> 	label does ASM_FIXUP_REGS and falls through to the local label).

Why do you need both labels? For each call, you know whether it is
local or nonlocal.

> 
> util/mkinit.c:
> 	Add NATIVE_GC to the list of defines where we need to run
> 	the label initialization code (we use the label table, like
> 	profiling does).

I don't understand this sentence.

> Index: library/array.m
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/library/array.m,v
> retrieving revision 1.38
> diff -u -r1.38 array.m
> --- array.m	1997/09/06 18:23:36	1.38
> +++ array.m	1997/09/18 07:22:40
> @@ -244,8 +244,11 @@
>  :- pragma(c_code, "
>  
>  Define_extern_entry(mercury____Unify___array__array_1_0);
> +MR_MAKE_STACK_LAYOUT_ENTRY(mercury____Unify___array__array_1_0);
>  Define_extern_entry(mercury____Index___array__array_1_0);
> +MR_MAKE_STACK_LAYOUT_ENTRY(mercury____Index___array__array_1_0);
>  Define_extern_entry(mercury____Compare___array__array_1_0);
> +MR_MAKE_STACK_LAYOUT_ENTRY(mercury____Compare___array__array_1_0);
>  
>  #ifdef  USE_TYPE_LAYOUT
>  
> Index: library/benchmarking.m
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/library/benchmarking.m,v
> retrieving revision 1.2
> diff -u -r1.2 benchmarking.m
> --- benchmarking.m	1997/08/05 04:10:00	1.2
> +++ benchmarking.m	1997/09/12 06:41:58
> @@ -130,6 +130,9 @@
>  Define_extern_entry(mercury__benchmarking__benchmark_nondet_5_0);
>  Declare_label(mercury__benchmarking__benchmark_nondet_5_0_i1);
>  Declare_label(mercury__benchmarking__benchmark_nondet_5_0_i2);
> +MR_MAKE_STACK_LAYOUT_ENTRY(mercury__benchmarking__benchmark_nondet_5_0);
> +MR_MAKE_STACK_LAYOUT_INTERNAL(mercury__benchmarking__benchmark_nondet_5_0, 
       1);
> +MR_MAKE_STACK_LAYOUT_INTERNAL(mercury__benchmarking__benchmark_nondet_5_0, 
       2);

Why an interleaved sequence in the first fragment and a batched sequence
in the second?

std_util:
> @@ -1153,11 +1162,17 @@
>  Define_extern_entry(mercury____Unify___std_util__univ_0_0);
>  Define_extern_entry(mercury____Index___std_util__univ_0_0);
>  Define_extern_entry(mercury____Compare___std_util__univ_0_0);
> -Declare_label(mercury____Compare___std_util__univ_0_0_i1);
> +MR_MAKE_STACK_LAYOUT_ENTRY(mercury____Unify___std_util__univ_0_0);
> +MR_MAKE_STACK_LAYOUT_ENTRY(mercury____Index___std_util__univ_0_0);
> +MR_MAKE_STACK_LAYOUT_ENTRY(mercury____Compare___std_util__univ_0_0);
> +MR_MAKE_STACK_LAYOUT_INTERNAL(mercury____Compare___std_util__univ_0_0, 1);

Did you intend to delete that label declaration? If yes, why is there
a MR_MAKE_STACK_LAYOUT_INTERNAL for it?

> RCS file: /home/staff/zs/imp/mercury/runtime/Mmakefile,v
>  HDRS		= calls.h conf.h context.h \
>  		  deep_copy.h dlist.h debug.h dummy.h \
>  		  engine.h getopt.h goto.h heap.h imp.h init.h label.h \
> -		  memory.h mercury_float.h mercury_grade.h \
> +		  memory.h mercury_accurate_gc.h mercury_float.h \
>  		  mercury_string.h mercury_trace.h mercury_trail.h \
>  		  mercury_types.h misc.h \
>  		  overflow.h prof.h prof_mem.h regorder.h regs.h \
>  		  spinlock.h std.h stacks.h \
>  		  table.h tags.h timing.h type_info.h wrapper.h \
>  		  $(LIBMER_DLL_H)

It is long past time we converted this mess to use one filename per line.
Similarly for other lists below.

> Index: runtime/goto.h
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/runtime/goto.h,v
> retrieving revision 1.34
> diff -u -r1.34 goto.h
> --- goto.h	1997/10/12 05:43:52	1.34
> +++ goto.h	1997/10/23 04:54:03
> @@ -12,37 +12,46 @@
>  #include "mercury_types.h"	/* for `Code *' */
>  #include "debug.h"		/* for debuggoto() */
>  
> +#define paste(a,b) a##b
> +#define stringify(string) #string

Don't we already have these defined somewhere else?

> Index: runtime/label.c
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/runtime/label.c,v
> retrieving revision 1.22
> diff -u -r1.22 label.c
> --- label.c	1997/08/28 17:52:31	1.22
> +++ label.c	1997/10/23 04:52:30
> @@ -50,15 +50,26 @@
>  }
>  
>  Label *
> -insert_entry(const char *name, Code *addr)
> +insert_entry(const char *name, Code *addr, Word *entry_layout_info)
>  {
>  	Label	*entry;
>  
>  	do_init_entries();
>  
> +
> +#ifdef MR_USE_STACK_LAYOUTS
> +	/* 
> +	** The succip will be set to the address stored in the
> +	** layout info. For some reason, this is different to
> +	** the address passed to insert_entry
> +	*/
> +	addr = entry_layout_info[0];
> +#endif /* MR_USE_STACK_LAYOUTS */
> +
>  	entry = make(Label);
>  	entry->e_name  = name;
>  	entry->e_addr  = addr;
> +	entry->e_layout  = entry_layout_info;

The comment is weird. I don't see any setting of succip, and I don't
see why the two definitions of addr should be different. This latter
matter must be resolved before we rely on layout tables.

> Index: runtime/label.h
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/runtime/label.h,v
> retrieving revision 1.14
> diff -u -r1.14 label.h
> --- label.h	1997/07/27 15:08:24	1.14
> +++ label.h	1997/08/27 08:20:53
> @@ -6,7 +6,10 @@
>  
>  /*
>  ** label.h defines the interface to the label table, which is a pair of
> -** hash tables mapping from procedure names to addresses and vice versa.
> +** hash tables, one mapping from procedure names and the other from
> +** addresses to label information.

"one mapping from procedure names" to what?

> +** The label information includes the name, address of the code, and
> +** layout information for that label.

The name of what?

>  #ifndef	LABEL_H
> @@ -18,10 +21,11 @@
>  typedef struct s_label {
>  	const char	*e_name;   /* name of the procedure	     */
>  	Code		*e_addr;   /* address of the code	     */
> +	Word		*e_layout; /* layout info for the procedure  */
>  } Label;

It ought to be easier to write C code that works with layout tables
if you define proper C structure types for the tables.

> +/*
> +** Definitions for MR_Code_Model
> +*/
> +
> +typedef enum { MR_CODE_MODEL_DET, MR_CODE_MODEL_NONDET } MR_Code_Model;

In the compiler, code_model has three possibilities. It would be better
to invent another name for this concept. Since its main use is to specify
which stack the procedure is using, I suggest something like
MR_stack_specifier.

Avoid the use of studlycaps in type names.

> +/*
> +** Definitions for MR_LIVE_LVAL

The following code does not define MR_LIVE_LVAL.

> +** MR_LIVE_LVAL describes an lval. This includes:
> +** 	- stack slots, registers, and special lvals such as succip, hp,
> +** 	  etc.
> +**
> +** The data is encoded using an 8 bit low tag, the rest of the word is a 
> +** data field describing which stack slot number or register number.
> +**
> +**  Lval		Tag	Rest
> +**  r(Num)		 0	Num
> +**  f(Num)		 1	Num
> +**  stackvar(Num)	 2	Num
> +**  framevar(Num)	 3	Num
> +**  succip		 4
> +**  maxfr		 5
> +**  curfr		 6
> +**  hp			 7
> +**  sp			 8
> +**  unknown		 9		(The location is not known)
> +**
> +** The type MR_Lval_Type describes the different tag values.
> +**
> +*/
> +
> +typedef enum { 
> +	MR_LVAL_TYPE_R,
> +	MR_LVAL_TYPE_F,
> +	MR_LVAL_TYPE_STACKVAR,
> +	MR_LVAL_TYPE_FRAMEVAR,
> +	MR_LVAL_TYPE_SUCCIP,
> +	MR_LVAL_TYPE_MAXFR,
> +	MR_LVAL_TYPE_CURFR,
> +	MR_LVAL_TYPE_HP,
> +	MR_LVAL_TYPE_SP,
> +	MR_LVAL_TYPE_UNKNOWN 
> +} MR_Lval_Type;
> +
> +#define MR_LIVE_LVAL_TAGBITS	8

Link this to the part of the compiler that also defines these, in both
directions.

> +/*
> +** Definitions for MR_LIVE_TYPE

The following code does not define MR_LIVE_TYPE.

> +**
> +** MR_LIVE_TYPE describes a live date. This includes:
> +** 	- succip, hp, curfr, maxfr, redoip, and
> +** 	  mercury data values (vars) that reference a type and an inst.

I don't know if I would call something a "date" if it included those things
:-)

> +** The data is encoded such that low values (less than
> +** TYPELAYOUT_MAX_VARINT) represent succip, hp, etc.  Higher values
> +** are pointers to a 2 cell, containing a type_info and an instantiation
> +** represention.
> +**
> +*/

2 cell -> 2 word cell

> +typedef enum { 
> +	MR_LIVE_TYPE_FRAMEVAR,
> +	MR_LIVE_TYPE_SUCCIP,
> +	MR_LIVE_TYPE_HP,
> +	MR_LIVE_TYPE_CURFR,
> +	MR_LIVE_TYPE_MAXFR,
> +	MR_LIVE_TYPE_REDOIP,
> +	MR_LIVE_TYPE_UNWANTED 
> +} MR_Lval_Type_NonVar;

What is MR_LIVE_TYPE_FRAMEVAR? Vars may be on the det stack, and the value
of MR_LIVE_TYPE_FRAMEVAR is less than TYPELAYOUT_MAX_VARINT, so I must
be missing something.

> +typedef struct { 
> +	Word	typeinfo;
> +	Word	inst;
> +} MR_Lval_Type_Var;

This has nothing to do with type variables or lvals. I suggest
MR_var_shape_info.

Either use field names typeinfo and instinfo, or type and inst.

> +/*
> +** Macros to support hand-written C code.
> +*/
> +
> +/*
> +** Define a stack layout for a label that you know very little about.
> +** It's just a generic entry label, no useful information, except
> +** the code address for the label.
> +*/ 
> +#ifdef MR_USE_STACK_LAYOUTS
> + #define MR_MAKE_STACK_LAYOUT_ENTRY(l) 				
       	\
> + const struct mercury_data__stack_layout__##l##_struct {		\
> +	Code * f1;							\
> +	Integer f2;							\
> +	Integer f3;							\
> +	Integer f4;							\
> + } mercury_data__stack_layout__##l = {				
       	\
> +	STATIC(l),							\
> +	(Integer) -1,	/* Unknown number of stack slots */		\
> +	(Integer) -1, 	/* Unknown code model */			\
> +        (Integer) MR_LVAL_TYPE_UNKNOWN 	/* Unknown succip location */
       	\
> + };
> +#else
> + #define MR_MAKE_STACK_LAYOUT_ENTRY(l)        
> +#endif	/* MR_USE_STACK_LAYOUTS */

It should be relatively easy to do better than this, with a macro that
takes two more arguments: a stack indicator and the stack frame size.
Succip will be stored in the bottom stack slot on the det stack, and
in a fixed slot on the nondet stack.

> +/*
> +** Define a stack layout for an internal label. Need to supply the
> +** label name (l) and the entry label name (e).
> +**
> +** The only useful information in this structure is the code address
> +** and the reference to the entry for this label.
> +*/ 

This macro and the following one are very similar, so add a comment that
explains the difference.

> +/*
> +** Macros to support stack layouts.
> +*/
> +#define MR_CONT_STACK_LAYOUT_GET_LABEL_ADDRESS(s)		\
> +		((Code *) field(0, (s), 0))
> +
> +#define MR_ENTRY_STACK_LAYOUT_GET_LABEL_ADDRESS(s)		\
> +		MR_CONT_STACK_LAYOUT_GET_LABEL_ADDRESS(s)
> +
> +#define MR_CONT_STACK_LAYOUT_GET_ENTRY_LAYOUT(s)		\
> +		(field(0, (s), 1))
> +
> +#define MR_ENTRY_STACK_LAYOUT_GET_NUM_SLOTS(s)			\
> +		(field(0, (s), 1))
> +
> +#define MR_ENTRY_STACK_LAYOUT_GET_CODE_MODEL(s)			\
> +		(field(0, (s), 2))
> +
> +#define MR_ENTRY_STACK_LAYOUT_GET_SUCCIP_LOC(s)			\
> +		(field(0, (s), 3))

Wouldn't it be easier to define a C struct for these?

About the renaming of the .mod files: we should aim to have all the standard
.h files that are referenced from compiler-generated .c files to have a
mercury_ prefix. This would require the corresponding source files to also
have the prefix. The rename now would be a chance to introduce this while
minimising disruption (for a changeover period, we should support both
forms of the header file names, with one file #including the other).

Zoltan.