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

[Tyson Richard DOWD]

Zoltan Somogyi wrote:
> 
> > 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.

In general you need two. If you find out for each label whether it is
local or nonlocal (quite possible to do) you can optimize a bit more. 

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

Add NATIVE_GC to the #ifdef .... that controls the running of the
initialization code for each module. NATIVE_GC requires the label
table to be initialized.

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

No. Good spotting.

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

Yes, I thought I was moving them, but I must have left them below
elsewhere as well.


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

Actually, studlycaps are our standard.

 4.3. Typedefs

    Use first letter uppercase for each word, other letters lowercase
    and underscores to separate words. For instance, Directory_Entry.

> > +/*
> > +** 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.

I assume you mean with comments.

> 
> > +/*
> > +** 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
> :-)

What if I threw in dinner and dancing?

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

That line shouldn't be there.

> > +/*
> > +** 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.

Yes, I expect to extend this at some time. I just don't want to
have to spend the time fixing it and forcing other people to maintain
it when there's nothing using it, and no easy way to test that you've
got it right.

> 
> > +/*
> > +** 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.
> 

Good idea.

> > +/*
> > +** 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?

Yep.

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

Well, if you want to do that, then I recommend doing it as a completely
separate change, to keep it as a nice clean break. The scheme for the
.h files is a good idea.

-- 
       Tyson Dowd           # To fix this, edit BLAH\BlahKey\Blah\Whatever 
                            # in the registry.
     trd at cs.mu.oz.au        # *WARNING* Editing the registry can DESTROY
http://www.cs.mu.oz.au/~trd # your machine forever. Do not do it.