[m-dev.] for review: deep profiling changes [part 2/3]

[Fergus Henderson]

On 26-Feb-2000, Thomas Conway <conway at cs.mu.OZ.AU> wrote:
> Index: compiler/mercury_compile.m
...
> +mercury_compile__maybe_compute_sccs(HLDS0, Verbose, Stats, HLDS) -->
> +	globals__io_lookup_bool_option(errorcheck_only, ErrorCheckOnly),
> +	globals__io_lookup_bool_option(profile_deep, DeepProfiling),
> +	(
> +		{ ErrorCheckOnly = no },
> +		{ DeepProfiling = yes }
> +	->
> +		maybe_write_string(Verbose, "% Computing SCC information...\n"),
> +		maybe_flush_output(Verbose),
> +		{ profiling__compute_scc_info(HLDS0, HLDS) },

Hmm, maybe_compute_sccs is called only from middle_pass,
and middle_pass is never called if errorcheck_only is `yes'.
So I don't think there is any need to check errorcheck_only here.

(The same comment applies to the existing code in inlining.m
which you probably cut-and-paste this code from.  I've just
fixed that.)

> Index: compiler/rl_code.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/rl_code.m,v
> retrieving revision 1.9
> diff -u -r1.9 rl_code.m
> --- compiler/rl_code.m	1999/09/07 05:48:54	1.9
> +++ compiler/rl_code.m	2000/02/25 23:39:25
> @@ -63,6 +63,7 @@
>  
>  :- type bytecode	--->
>  		rl_EXP_bool_immed(int32)
> +/*
>  	;	rl_EXP_int_immed(int32)
>  	;	rl_EXP_bool_push(int32)
>  	;	rl_EXP_int_push(int32)
> @@ -419,6 +420,7 @@
>  	;	rl_HEAD_var_stream(int32)
>  	;	rl_HEAD_define_rule(int32,int32,int32)
>  	;	rl_HEAD_last_bytecode
> +*/
>  	.
>  
>  
> @@ -445,6 +447,7 @@
>  	int16_to_bytecode(0, I0Codes),
>  	int32_to_bytecode(X0int32, X0int32Codes),
>  	list__condense([I0Codes,X0int32Codes], Splits).
> +/*
>  bytecode_to_intlist(rl_EXP_int_immed(X0int32),	 Splits) :-
>  	int16_to_bytecode(1, I1Codes),
>  	int32_to_bytecode(X0int32, X0int32Codes),
> @@ -1714,6 +1717,7 @@
>  bytecode_to_intlist(rl_HEAD_last_bytecode,	 Splits) :-
>  	int16_to_bytecode(356, I356Codes),
>  	list__condense([I356Codes], Splits).
> +*/
>  
>  int32_to_bytecode(X, List) :-
>  	int32_to_byte_list(X, List).

Uh, I think the Aditi group will not be very happy if you
commit those changes... was that intentional?

> Index: compiler/stack_layout.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/stack_layout.m,v
> retrieving revision 1.43
> diff -u -r1.43 stack_layout.m
> --- compiler/stack_layout.m	2000/01/14 01:10:43	1.43
> +++ compiler/stack_layout.m	2000/02/25 23:39:36
> @@ -46,6 +46,7 @@
>  %	predicate name		(String)
>  %	predicate arity		(int_least16_t)
>  %	procedure number	(int_least16_t)
> +%	owner scc id		(Word) actually MR_SCCId *
>  %
>  % Automatically generated unification, index and comparison predicates
>  % use the second form:
> @@ -56,6 +57,7 @@
>  %	predicate name		(String)
>  %	predicate arity		(int_least16_t)
>  %	procedure number	(int_least16_t)
> +%	owner scc id		(Word) actually MR_SCCId *

Using an integer type to hold a pointer in something that
will be statically initialized is not portable.
If you do need to use some generic type, you should use a
pointer type such as `void *' or `Word *'.

> @@ -270,6 +272,7 @@
>  	module_info_name(ModuleInfo0, ModuleName),
>  	module_info_get_cell_count(ModuleInfo0, CellCount),
>  	module_info_globals(ModuleInfo0, Globals),
> +	% module_info_get_scc_info(ModuleInfo0, scc_info(ProcSCCs, _)),

You should either delete that commented-out line of code,
or add a comment explaining why the code is commented out.

> -:- pred stack_layout__construct_procid_rvals(proc_label::in,
> +:- pred stack_layout__construct_procid_rvals(proc_label::in, maybe(rval)::in,
>  	list(maybe(rval))::out, initial_arg_types::out) is det.
>  
> -stack_layout__construct_procid_rvals(ProcLabel, Rvals, ArgTypes) :-
> +stack_layout__construct_procid_rvals(ProcLabel, SCCId0, Rvals, ArgTypes) :-
> +	(
> +		SCCId0 = yes(_),
> +		SCCIdElem = [SCCId0],
> +		SCCIdElemType = [1 - no]

I think the `no' there will need to change,
if you change the type from `Word' (see my earlier comment).

> @@ -784,9 +792,10 @@
>  				yes(const(string_const(DefModuleString))),
>  				yes(const(string_const(PredName))),
>  				yes(const(int_const(Arity))),
> -				yes(const(int_const(Mode)))
> +				yes(const(int_const(Mode)))|
> +				SCCIdElem
>  			],
> -		ArgTypes = [4 - no, 2 - yes(int_least16)]
> +		ArgTypes = [4 - no, 2 - yes(int_least16)|SCCIdElemType]

s/|/ | /

>  	;
>  		ProcLabel = special_proc(DefModule, PredName, TypeModule,
>  			TypeName, Arity, ProcId),
> @@ -799,9 +808,10 @@
>  				yes(const(string_const(DefModuleString))),
>  				yes(const(string_const(PredName))),
>  				yes(const(int_const(Arity))),
> -				yes(const(int_const(Mode)))
> +				yes(const(int_const(Mode)))|
> +				SCCIdElem

s/|/ |/

>  			],
> -		ArgTypes = [4 - no, 2 - yes(int_least16)]
> +		ArgTypes = [4 - no, 2 - yes(int_least16)|SCCIdElemType]

s/|/ | /

> +stack_layout__construct_closure_layout(UsingStackLayouts, ProcLabel, SCCId,
> +		ClosureLayoutInfo, Rvals, ArgTypes, CNum0, CNum) :-
...
> +		ProcIdRval = create(0, ProcIdRvals,
> +			initial(ProcIdTypes, none), must_be_static,
> +			CNum0, "proc_layout_vector", no),
> +		CNum1 = CNum0 + 1

You should double-check that you are using the same
convention with regard to ++CNum vs CNum++
as the rest of the code.

Hmm, I just checked it myself, and it looks to me like
you are using the opposite convention.
It would be nice to use the `counter' type that Zoltan
just posted for review here.  But that should be a
separate change.  For now, just make sure you use
`CNum' rather than `CNum0' in the arguments of create/7.

> +	Rvals = [yes(ProcIdRval)|LayoutRvals],

s/|/ | /

> Index: compiler/unify_gen.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/unify_gen.m,v
> retrieving revision 1.104
> diff -u -r1.104 unify_gen.m
> --- compiler/unify_gen.m	2000/01/14 01:10:46	1.104
> +++ compiler/unify_gen.m	2000/02/25 23:39:38
> @@ -41,6 +41,7 @@
>  :- import_module hlds_module, hlds_pred, prog_data, prog_out, code_util.
>  :- import_module mode_util, type_util, code_aux, hlds_out, tree, arg_info.
>  :- import_module globals, options, continuation_info, stack_layout.
> +:- import_module profiling.
>  
>  :- import_module term, bool, string, int, list, map, require, std_util.
>  
> @@ -530,8 +531,13 @@
>  		{ code_util__extract_proc_label_from_code_addr(CodeAddr,
>  			ProcLabel) },
>  		code_info__get_cell_count(CNum0),
> -		{ stack_layout__construct_closure_layout(ProcLabel,
> -			ClosureInfo, ClosureLayoutMaybeRvals,
> +		profiling__scc_id(proc(PredId, ProcId), SCCId),
> +		code_info__get_globals(Globals),
> +		{ globals__lookup_bool_option(Globals, procid_stack_layout,
> +			StackLayouts) },
> +		{ stack_layout__construct_closure_layout(StackLayouts,
> +			ProcLabel, SCCId, ClosureInfo,
> +			ClosureLayoutMaybeRvals,
>  			ClosureLayoutArgTypes, CNum0, CNum) },

Hmm, shouldn't (at least part of) that be inside a test
to see whether the deep_profiling option is set?
What will profiling__scc_id do if we haven't computed the SCCs?

> @@ -190,6 +193,7 @@
>  vn_type__vnrval_type(vn_stackvar_addr(_), data_ptr).
>  vn_type__vnrval_type(vn_framevar_addr(_), data_ptr).
>  vn_type__vnrval_type(vn_heap_addr(_, _, _), data_ptr).
> +vn_type__vnrval_type(vn_c_func(_, _, _, _), word).

Shouldn't that use the return type lval_type field of the
vn_c_func functor, rather than `word'?

> Index: library/array.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/library/array.m,v
> retrieving revision 1.67
> diff -u -r1.67 array.m
> --- library/array.m	2000/01/19 09:45:16	1.67
> +++ library/array.m	2000/02/25 23:39:49
> @@ -276,6 +276,26 @@
>  Declare_entry(mercury__array__array_equal_2_0);
>  Declare_entry(mercury__array__array_compare_3_0);
>  
> +#ifdef MR_PROFILE_DEEP
> +  MR_MAKE_SCC_ID(unify_array_scc_id, { }, { }, { });
> +  MR_MAKE_PROC_LAYOUT(mercury____Unify___array__array_1_0,
> +  	MR_DETISM_SEMI, MR_ENTRY_NO_SLOT_COUNT, MR_LONG_LVAL_TYPE_UNKNOWN,
> +	MR_PREDICATE, ""array"", ""builtin_compare"", 2, 0,
> +	unify_array_scc_id);
> +
> +  MR_MAKE_SCC_ID(index_array_scc_id, { }, { }, { });
> +  MR_MAKE_PROC_LAYOUT(mercury____Index___array__array_1_0,
> +  	MR_DETISM_DET, MR_ENTRY_NO_SLOT_COUNT, MR_LONG_LVAL_TYPE_UNKNOWN,
> +	MR_PREDICATE, ""array"", ""builtin_index"", 2, 0,
> +	index_array_scc_id);
> +
> +  MR_MAKE_SCC_ID(compare_array_scc_id, { }, { }, { });
> +  MR_MAKE_PROC_LAYOUT(mercury____Compare___array__array_1_0,
> +  	MR_DETISM_DET, MR_ENTRY_NO_SLOT_COUNT, MR_LONG_LVAL_TYPE_UNKNOWN,
> +	MR_PREDICATE, ""array"", ""builtin_index"", 3, 0,
> +	compare_array_scc_id);
> +#endif

Pretty much the same code appears for the unification/index/compare
predicates for array__array, pred, c_pointer, private_builtin__type_info,
private_builtin__typeclass_info, std_util__univ, and std_util__type_info.
I think it would be worth defining a single macro that would expand to 
this code, rather than duplicating the code seven times.

> Index: runtime/mercury_ho_call.c
...
> +#ifdef MR_PROF_DEEP
> +MR_MAKE_HO_CALL_SITE(genu_to_specu, mercury__unify_2_0, "runtime", 334);
> +MR_MAKE_SCC_ID(unify_scc_id, { }, { &genu_to_specu }, { });
> +MR_MAKE_PROC_LAYOUT(mercury__unify_2_0, MR_DETISM_SEMI, 0, 0,
> +			MR_PREDICATE, "builtin", "unify", 2, 0,
> +			unify_scc_id);
> +
> +MR_MAKE_HO_CALL_SITE(geni_to_speci, mercury__index_2_0, "builtin", 574);

What are the magic numbers 334, 574, etc. here?

> @@ -268,13 +318,59 @@
>  				restore_registers();
>  			}
>  
> +#ifndef MR_PROFILE_DEEP
>  			tailcall(type_ctor_info->unify_pred,
>  				LABEL(mercury__unify_2_0));
> +#else
> +{
> +			/*
> +			** When doing deep profiling, we need to
> +			** make a stack frame and treat the call
> +			** the way user HO calls get treated,
> +			** except that instead of using an MR_Closure
> +			** we use the 
> +			*/

That comment is incomplete.

(This occurs in several places.)

> Index: runtime/mercury_prof.c
> @@ -111,6 +114,9 @@
>  ** Local function declarations
>  */
>  
> +void
> +checked_atexit(void (*func)(void));

If that function is local, it should be declared static.
If it is not, then it should be declared in the header
file rather than in the .c file.

> Index: runtime/mercury_stack_layout.h
...
>  #define MR_MAKE_PROC_LAYOUT(entry, detism, slots, succip_locn,		\
> -		pf, module, name, arity, mode) 				\
> -	MR_Stack_Layout_Entry mercury_data__layout__##entry = {		\
> +		pf, module, name, arity, mode, sccd)			\
> +	const struct mercury_data__layout__##entry##_struct { 		\
> +		/* stack traversal group */				\
> +		Code			*MR_sle_code_addr;		\
> +		MR_Long_Lval		MR_sle_succip_locn;		\
> +		MR_int_least16_t	MR_sle_stack_slots;		\
> +		MR_Determinism		MR_sle_detism;			\
> +		/* proc id group */					\
> +		MR_Stack_Layout_Proc_Id MR_sle_proc_id;			\
> +	} mercury_data__layout__##entry = {				\
>  		MR_MAKE_PROC_LAYOUT_ADDR(entry),			\
>  		succip_locn,						\
>  		slots,							\

Could you explain that change for me?
I don't understand why you can't use MR_Stack_Layout_Entry.

> Index: runtime/mercury_type_info.h
...
> @@ -848,6 +848,11 @@
>  	String				type_ctor_module_name;
>  	String				type_ctor_name;
>  	Integer				type_ctor_version;
> +#ifdef MR_PROFILE_DEEP
> +	const Word			*unify_info;
> +	const Word			*index_info;
> +	const Word			*compare_info;
> +#endif

It would be a good idea to have some comments here explaining the
meaning and format of the data that these pointers point to.

> @@ -882,7 +892,16 @@
>  		NULL,							\
>  		MR_string_const(MR_STRINGIFY(m), sizeof(MR_STRINGIFY(m))-1),\
>  		MR_string_const(MR_STRINGIFY(n), sizeof(MR_STRINGIFY(n))-1),\
> -		MR_RTTI_VERSION						\
> +		MR_RTTI_VERSION,					\
> +	MR_IF_PROFILE_DEEP(						\
> +		(const Word *) MR_REF_SLEc(u)				\
> +	)								\
> +	MR_IF_PROFILE_DEEP(						\
> +		(const Word *) MR_REF_SLEc(i)				\
> +	)								\
> +	MR_IF_PROFILE_DEEP(						\
> +		(const Word *) MR_REF_SLEc(c)				\
> +	)								\

I think it would be clearer to use a single invocation of
MR_IF_PROFILE_DEEP() here.  Also, that code looks buggy:
isn't it missing some commas between the different items?

-- 
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.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------