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

[Fergus Henderson]

On 26-Feb-2000, Thomas Conway <conway at cs.mu.OZ.AU> wrote:
> Outstanding issues:
> 	This change does not include the browser for deep profiles.

Hmm... a browser would certainly be useful.

Do you have any test cases?
Without a browser of some kind, it seems difficult
to define any meaningful test cases, and without
test cases, a change of this magnitude is very likely
to suffer code rot over time.  So it would certainly
be nice to 

> 	Typeclasses are not fully addressed.

What issues arise for deep profiling of code using typeclasses?

> 	In the absence of a browser, I haven't added to the NEWS file
> 	yet.

You should add it to the WORK_IN_PROGRESS file then.

> compiler/*.m:
> 	Add a new lvalue `global/2' which denotes a global variable.
> 
> 	Add a new rvalue `c_func/4' which denotes a call to a function in C.
> 
> 	If deep profiling is enabled, add a pointer to the MR_SCCId in the
> 	MR_Stack_Layout_Entry structures, and add pointers to the
> 	MR_Stack_Layout_Entry structures for the unify, index and compare
> 	preds in the type_ctor_layout strcutures.
> 
> 	If we are in a grade that uses stack layouts, then include a
> 	pointer to the MR_Stack_Layout_Proc_Id generated for the procedure
> 	in any closure layouts we construct, rather than making one as we
> 	go. This is necessary with deep profiling enabled because otherwise
> 	we can't get our hands on the correct MR_SCCId for the closure.
> 
> 	Generate static MR_SCCId structures which include the info about
> 	the call-sites in the SCC.
> 
> 	Modify the code generator to emit updates of the
> 	MR_prof_current_scc and MR_prof_current_proc pointers before
> 	and after calls, and in procedure prologues and epilogues.
> 
> 	Modify the code generator so that when we export procedures, we
> 	update the deep profiling pointers appropriately.

It would be helpful if you could be more specific about
identifying which files were affected by these changes.
For files which have had only minor changes, it's OK to
use wild-cards rather than explicitly listing every such file,
but the log message really ought to explicitly identify
any files which have had significant changes.

You should add a definition of "SCC" to notes/glossary.html.

> runtime/mercury_ho_call.h:
> 	Make the MR_Stack_Layout_Proc_Id member of MR_Closure_Layout
> 	be a pointer rather than a member.

What's the rationale for that?

> runtime/mercury_type_info.h:
> 	Add pointers to the MR_Stack_Layout_Entry structures for unify,
> 	index and compare to MR_TypeCtorInfo.

Could you explain why that is needed?

> runtime/mercury_wrapper.c:
> 	Add hand-written deep profiling structures for do_interpreter, and
> 	modify the code to support deep profiling.
> 	Add program_entry_layout - a pointer to the MR_Stack_Layout_Entry
> 	for the program entry point.
> 
> 
> Index: compiler/base_type_info.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/base_type_info.m,v
> retrieving revision 1.32
> diff -u -r1.32 base_type_info.m
> --- compiler/base_type_info.m	2000/02/08 06:59:23	1.32
> +++ compiler/base_type_info.m	2000/02/25 23:37:58
> @@ -158,6 +158,20 @@
>  		_Status, Elim, Procs, HldsDefn),
>  	base_type_info__construct_pred_addrs(Procs, Elim, ModuleInfo, 
>  		PredAddrArgs),
> +
> +	module_info_globals(ModuleInfo, Globals),
> +	globals__lookup_bool_option(Globals, profile_deep, DeepProfiling),
> +	( DeepProfiling = yes ->
> +		map((pred(proc(PredId, ProcId)::in, ProcLayout::out) is det :-
> +		    code_util__make_local_entry_label(ModuleInfo,
> +				PredId, ProcId, no, Entry),
> +		    ProcLayout = yes(const(data_addr_const(
> +		    		data_addr(ModuleName, proc_layout(Entry)))))
> +		), Procs, ProcLayouts)

s/map/list__map/

A comment here explaining what this code is doing (and why)
would be helpful.

Also it would be more concise and perhaps a tiny bit clearer
to use the function version of list__map.

> Index: compiler/call_gen.m
...
> +		% If deep profiling is enabled, then we need to
> +		% emit code to update the position pointers, before
> +		% and after (down below) the call.

What are these "position pointers"?
Probably these are documented somewhere else.
It would be helpful to give a pointer to that
documentation here, i.e.
s/position pointers/position pointers (see ...)/

> @@ -185,6 +211,19 @@
>  		% the immediate arguments twice.
>  	call_gen__generic_call_setup(GenericCall, InVars, OutVars, SetupCode),
>  
> +	code_info__get_globals(Globals),
> +	{ globals__lookup_bool_option(Globals, profile_deep, DeepProfiling) },
> +	{ goal_info_get_context(GoalInfo, Context) },
> +	(
> +		{ DeepProfiling = yes },
> +		{ ClosureRval = lval(reg(r, 1)) },
> +		profiling__pre_ho_call_update(ClosureRval, Context,
> +			ProfilingEntryCode)
> +	;
> +		{ DeepProfiling = no },
> +		{ ProfilingEntryCode = empty }
> +	),

I suggest you duplicate the comment above here too.

> Index: compiler/code_gen.m
...
> +		% If we're doing deep profiling, we need to reserve
> +		% stack slots for saving the current proc and current scc.
> +	globals__lookup_bool_option(Globals, profile_deep, ProfDetail),
> +	( ProfDetail = yes ->
> +		profiling__setup(CodeInfo0, CodeInfo1)
> +	;
> +		CodeInfo1 = CodeInfo0
> +	),

I suggest s/ProfDetail/DeepProfiling/g
(or perhaps s/ProfDetail/ProfileDeep/g).

>  		}
> -	;
> +	; % XXX ProfD = yes -> ....

You should explain that XXX comment
(this occurs in several places).

> Index: compiler/code_info.m
...
> @@ -310,6 +326,14 @@
>  				% which would make it impossible to describe
>  				% to gc what the slot contains after the end
>  				% of the branched control structure.
> +		profiling_info :: maybe(profiling_info),
> +				% Information about where in the stack frame
> +				% for the current procedure, the data for
> +				% updating the accurate profiling structures
> +				% is stored.
> +		scc_info :: scc_info,
> +				% Information about SCCs, etc used for
> +				% accurate profiling.

s/accurate profiling/deep profiling/g  ?

I think it would be good to be consistent about the terminology.

> @@ -595,6 +630,18 @@
>  :- pred code_info__succip_is_used(code_info, code_info).
>  :- mode code_info__succip_is_used(in, out) is det.
>  
> +:- type profiling_info
> +	--->	profiling_info(
> +			lval,	% current scc_id slot
> +			lval	% current proc_num slot
> +		).

It would be helpful here to have some documentation
about how this type is used.

If this is only needed for deep profiling, then
perhaps s/profiling_info/deep_profiling_info/g ?

> @@ -2328,12 +2387,16 @@
>  	% point), or that it would be more efficient to put the two labels
>  	% in the other order (e.g. because the code after the resumption point
>  	% needs most of the variables in their stack slots).
> +	%
> +	% If deep profiling is enabled, we need to reset both the current
> +	% proc (in case we get a SIG_PROF) and the current scc, so that
> +	% subsquent calls get hung off the right point.

s/subsquent/subsequent/
                ^

Also it might be a bit clearer if you change
"reset ..." to "reset the global variables holding ..."

> Index: compiler/code_util.m
> @@ -321,7 +330,7 @@
>  			Module = ModuleName
>  		),
>  		ProcLabel = special_proc(Module, "__Unify__", TypeModule,
> -			TypeName, Arity, UniModeNum)
> +			TypeName, Arity, UniModeNum) % XXX

You should explain that XXX.

>  	;
>  		error("code_util__make_uni_label: unqualified type_id")
>  	).
> @@ -952,7 +961,14 @@
>  	list__append(Lvals1, Lvals2, Lvals).
>  code_util__lvals_in_rval(mem_addr(MemRef), Lvals) :-
>  	code_util__lvals_in_mem_ref(MemRef, Lvals).
> +code_util__lvals_in_rval(c_func(_, _, Args, _), Lvals) :-
> +	list__map(lambda([Arg::in, ArgLvals::out] is det, (
> +		Arg = _Type - Rval,
> +		code_util__lvals_in_rval(Rval, ArgLvals)
> +	)), Args, LvalsList),
> +	list__condense(LvalsList, Lvals).
>  
> +
>  code_util__lvals_in_lval(reg(_, _), []).
>  code_util__lvals_in_lval(stackvar(_), []).
>  code_util__lvals_in_lval(framevar(_), []).
> @@ -977,6 +993,7 @@
>  	list__append(Lvals1, Lvals2, Lvals).
>  code_util__lvals_in_lval(lvar(_), []).
>  code_util__lvals_in_lval(temp(_, _), []).
> +code_util__lvals_in_lval(global(_, _), []).
>  code_util__lvals_in_lval(mem_ref(Rval), Lvals) :-
>  	code_util__lvals_in_rval(Rval, Lvals).
>  
> @@ -987,5 +1004,29 @@
>  code_util__lvals_in_mem_ref(framevar_ref(_), []).
>  code_util__lvals_in_mem_ref(heap_ref(Rval, _, _), Lvals) :-
>  	code_util__lvals_in_rval(Rval, Lvals).
> +
> +%-----------------------------------------------------------------------------%
> +
> +code_util__make_proc_layout_ref(PPId, ModuleInfo, Rval) :-
> +	PPId = proc(PredId, ProcId),
> +	code_util__make_proc_label(ModuleInfo, PredId, ProcId,
> +			ProcLabel),
> +	pred_module(PPId, ModuleInfo, ModuleName),
> +	Rval = const(data_addr_const(data_addr(ModuleName,
> +			proc_layout(local(ProcLabel))))).
> +
> +%-----------------------------------------------------------------------------%
> +
> +code_util__stackref_to_string(Lval, LvalStr) :-
> +	( Lval = stackvar(Slot) ->
> +		string__int_to_string(Slot, SlotString),
> +		string__append_list(["detstackvar(", SlotString, ")"], LvalStr)
> +	; Lval = framevar(Slot) ->
> +		string__int_to_string(Slot, SlotString),
> +		string__append_list(["framevar(", SlotString, ")"], LvalStr)
> +	;
> +		error("non-stack lval in stackref_to_string")
> +	).
> +

You should use the MR_framevar() and MR_detstackvar() macros.
Note that framevar() numbers slots from zero while MR_framevar()
numbers slots from one, so you need to be careful to make sure
you don't get an off-by-one error there.  A test case
would be very helpful.

> Index: compiler/continuation_info.m
> @@ -732,12 +739,14 @@
>  continuation_info__live_value_type(lval(reg(_, _)), unwanted).
>  continuation_info__live_value_type(lval(stackvar(_)), unwanted).
>  continuation_info__live_value_type(lval(framevar(_)), unwanted).
> +continuation_info__live_value_type(lval(global(_, _)), unwanted).	% XXX

You should explain that XXX.

> Index: compiler/dependency_graph.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/dependency_graph.m,v
> retrieving revision 1.45
> diff -u -r1.45 dependency_graph.m
> --- compiler/dependency_graph.m	2000/02/21 01:41:29	1.45
> +++ compiler/dependency_graph.m	2000/02/25 23:38:22
...
> -:- type scc_id == int.
> +:- type aditi_scc_id == int.

That change should be mentioned in the log message, IMHO.

> Index: compiler/handle_options.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/handle_options.m,v
> retrieving revision 1.88
> diff -u -r1.88 handle_options.m
> --- compiler/handle_options.m	2000/01/10 05:26:18	1.88
> +++ compiler/handle_options.m	2000/02/25 23:38:33
> @@ -368,6 +368,7 @@
>  
>  	% Deep profiling requires `procid' stack layouts
>  	option_implies(profile_deep, procid_stack_layout, bool(yes)),
> +	option_implies(profile_deep, middle_rec, bool(no)),

The comment there does not apply to the line of code you've
added, so I'd suggest at least leaving a blank line between
the line you added and the one before it.
Even better would be to add a comment explaining why
profile_deep needs to disable middle_rec.

>  	% --no-reorder-conj implies --no-deforestation.
>  	option_neg_implies(reorder_conj, deforestation, bool(no)),
> Index: compiler/hlds_module.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/hlds_module.m,v
> retrieving revision 1.50
> diff -u -r1.50 hlds_module.m
> --- compiler/hlds_module.m	2000/01/13 06:15:41	1.50
> +++ compiler/hlds_module.m	2000/02/25 23:38:33
> @@ -86,6 +86,43 @@
>  	% map from proc to a list of unused argument numbers.
>  :- type unused_arg_info == map(pred_proc_id, list(int)).
>  
> +:- type scc_info
> +	--->	scc_info(
> +			map(pred_proc_id, scc_id),
> +				% To which scc does each proc belong?
> +				% Leaf procedures are not assigned an
> +				% scc_id.
> +			map(scc_id, scc_data)
> +				% for each scc, keep track of call site
> +				% numbers for first order calls,
> +				% higher order calls, and class method
> +				% calls, and store the info about each
> +				% call site for the static SCCId structures.
> +		).

A comment here at the top of the type declaration explaining what the
scc_info as a whole is used for would be helpful; for example,
perhaps something like

	% The scc_info stores information about the call graph,
	% specifically how it is divided into strongly connected
	% components (SCCs), that we need for deep profiling.
	% See profiling.m for more information.

(presuming of course that there actually is some more
information in profiling.m).

> +:- type scc_data
> +	--->	scc_data(
> +			int,
> +			list(first_order_call),
> +			int,
> +			list(higher_order_call),
> +			int,
> +			list(class_method_call),
> +			may_call_mercury
> +		).

You need some comments here explaining what the
various fields are supposed to represent.

I think perhaps the comment in the scc_info type

				% for each scc, keep track of call site
				% numbers for first order calls,
				% higher order calls, and class method
				% calls, and store the info about each
				% call site for the static SCCId structures.

might be trying to explain what information is
stored in the scc_data type, but if so, it is
not clear; that part of the comment should be
moved so that it is clear that it refers to the
scc_data type and so it is clear which field is
which and what each field represents.

Are the ints each just equal to the length of the lists
that follow them?  If so, is there any need to store
them, rather than computing them on demand?

> +:- type first_order_call
> +	--->	first_order_call(pred_proc_id, pred_proc_id, term__context).
> +
> +:- type higher_order_call
> +	--->	higher_order_call(pred_proc_id, term__context).
> +
> +:- type class_method_call
> +	--->	class_method_call(pred_proc_id, pred_proc_id, term__context).

It's not clear what the fields here are supposed to represent;
probably the caller and the callee, at a guess, but I've no
idea which is which.  I suggest you use record syntax.

These types all include a term__context field.
That should probably be prog_context rather than term__context.

> +:- type scc_id
> +	--->	scc_id(module_name, int).

What is this type for?
What is the int here?

> Index: compiler/hlds_pred.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/hlds_pred.m,v
> retrieving revision 1.72
> diff -u -r1.72 hlds_pred.m
> --- compiler/hlds_pred.m	2000/02/16 02:15:36	1.72
> +++ compiler/hlds_pred.m	2000/02/25 23:38:39
> @@ -71,6 +71,12 @@
>  :- pred invalid_proc_id(proc_id).
>  :- mode invalid_proc_id(out) is det.
>  
> +:- pred pred_has_local_code(pred_proc_id, module_info).
> +:- mode pred_has_local_code(in, in) is semidet.

You should document what you mean by "has local code".

Also, since that takes a pred_proc_id, and acts on the
procedure rather than the predicate, shouldn't it be named
`proc_has_local_code'?

> +:- pred pred_module(pred_proc_id, module_info, module_name).
> +:- mode pred_module(in, in, out) is det.
...
> +pred_module(proc(PredId, _ProcId), ModuleInfo, Name) :-
> +	module_info_pred_info(ModuleInfo, PredId, PredInfo),
> +	pred_info_module(PredInfo, Name).

It would be simpler to implement that as

pred_module(proc(PredId, _ProcId), ModuleInfo, Name) :-
	predicate_module(ModuleInfo, PredId, Name).

But that predicate is in fact so simple that I think
it would be better to just write it inline rather than
defining it as a separate subroutine.  I don't think
that is a useful abstraction.

>  status_is_exported(imported(_),			no).
>  status_is_exported(abstract_imported,		no).
> Index: compiler/inlining.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/inlining.m,v
> retrieving revision 1.84
> diff -u -r1.84 inlining.m
> --- compiler/inlining.m	1999/10/25 03:48:59	1.84
> +++ compiler/inlining.m	2000/02/25 23:38:41
> @@ -265,7 +265,8 @@
>  			{ NumUses = 1 }
>  		),
>  		% Don't inline recursive predicates
> -		{ \+ goal_calls(CalledGoal, PredProcId) },
> +		{ PredProcId = proc(PredId, _ProcId) },
> +		{ \+ goal_calls_pred_id(CalledGoal, PredId) },

I don't think that was mentioned in the log message.
Can you explain that change?
If one mode of a predicate calls a different mode of the
same predicate, it ought to be OK to inline that procedure,
shouldn't it?

> +++ compiler/llds.m	2000/02/25 23:38:51
> @@ -129,7 +129,12 @@
>  			proc_label		% The id of the procedure
>  						% whose table this variable
>  						% represents.
> -		).
> +		)
> +	;	scc_id_counter(
> +			module_name,
> +			int
> +		)
> +	.

A comment here would be helpful.

> @@ -645,6 +650,9 @@
>  	;	sp		% Virtual machine register point to the
>  				% top of det stack.
>  
> +	;	global(llds_type, string)
> +				% A global variable

What's the string?
Is that the complete name of the global variable?
Would it make more sense to use a module-qualified name?

> +	;	c_func(
> +			llds_type,			% return type
> +			string,				% name
> +			list(pair(llds_type, rval)),	% arguments
> +			staticly_evaluable		% static or dynamic?
> +		)
> +		% calls some arbitary C function with the given rvals as
> +		% arguments.
> +	.

s/arbitary/arbitrary/

Is the function call allowed to have side-effects?

If so, that may cause problems, I think, e.g. for value numbering.
If not, this should be explicitly documented.

> +	% For c_func rvals, staticly_evaluable tells the compiler whether
> +	% the value of the function can be determined at compile time, or
> +	% whether it needs to be defered till runtime.
> +:- type staticly_evaluable
> +	--->	static
> +	;	dynamic
> +	.

s/staticly/statically/g
s/defered/deferred/

The comment there is not clear.  Determined by whom?  By the user?
By the Mercury compiler? By the C compiler?

If this field is supposed to indicate whether the rval can be used
in a C static initializer, then you should say so.
(The only time this could be the case would be if the C
function is actually a macro; that is worth mentioning
in the comments too.)

> +++ compiler/llds_out.m	2000/02/25 23:39:00
> @@ -784,6 +785,15 @@
>  	{ DataAddr = data_addr(ModuleName, tabling_pointer(ProcLabel)) },
>  	{ decl_set_insert(DeclSet0, data_addr(DataAddr), DeclSet) }.
>  
> +output_comp_gen_c_var(scc_id_counter(ModuleName, SCCNum),
> +		DeclSet, DeclSet) -->
> +	{ llds_out__sym_name_mangle(ModuleName, MangledModuleName) },
> +	io__write_string("\nInteger mercury_var__"),
> +	io__write_string(MangledModuleName),
> +	io__write_string("__scc_id__"),
> +	io__write_int(SCCNum),
> +	io__write_string(" = 0;\n").

`mercury_var__' is used in other places too, I think;
it would be good to abstract that out into a function.

> @@ -2642,6 +2669,7 @@
>  output_lval_decls(sp, _, _, N, N, DeclSet, DeclSet) --> [].
>  output_lval_decls(lvar(_), _, _, N, N, DeclSet, DeclSet) --> [].
>  output_lval_decls(temp(_, _), _, _, N, N, DeclSet, DeclSet) --> [].
> +output_lval_decls(global(_, _), _, _, N, N, DeclSet, DeclSet) --> [].

Don't you need to output a declaration for the global variable?

> +output_static_rval(c_func(_RT, Name, Args, Static)) -->
> +	(
> +		{ Static = static },
> +		write_string(Name),
> +		write_string("("),
> +		output_static_rvals(Args),
> +		write_string(")")
> +	;
> +		{ Static = dynamic },
> +		{ error("Cannot output a c_func/4 in a static initializer") }
> +	).

I suggest  s/output a c_func/output a dynamic c_func/
otherwise the message is slightly misleading.

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