[m-rev.] Updated diff for deep profiling.

[Fergus Henderson]

On 17-May-2001, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> The main documentation on the general architecture of the deep profiler
> is the deep profiling paper.

There is one problem with documenting things in papers, namely that the
documentation in papers doesn't get maintained when the software changes.

I think it would be a good idea to have at least a brief summary
of the general architecture somewhere in the documentation that
goes along with the code.  I also think it would be nice to have
an overview of the deep profiler sources, at about the level of
compiler/notes/compiler_design.html; copying the relevant parts of the
log message into a text file called say deep/README or deep/OVERVIEW
would be a good start.

> deep/*

I think it would be much clearer to name the directory "deep_profiler"
rather than "deep".  A user who downloads the source, unpacks it, and does
"ls" is unlikely to know what "deep" means in this context.

> compiler/options.m:
> 	Add options to turn on deep profiling and (for benchmarking purposes)
> 	control its implementation.
> 
> 	Add an optimize to disable tailcall optimization in the LLDS backend,
> 	to help benchmarking deep profiling.

s/an optimize/an option/?

> compiler/layout_out.m:
> compiler/llds_out.m:
> 
> compiler/code_util.m:
> 	Make code_util__make_proc_label_from_rtti a function, and export it.

What changes were made to layout_out.m and llds_out.m?

> Index: Mmakefile
...
> +cleanint:
> +	for dir in browser compiler deep library profiler; do \
> +		echo looking for inappropriate files in the $$dir directory: ; \
> +		( cd $$dir; cleanint > .cleanint ) ; \
> +		if test -s $$dir/.cleanint ; then \
> +			cat $$dir/.cleanint ; \
> +		else \
> +			echo none found. ; \
> +		fi \
> +	done

- You should document what this target does.
- This target uses the command "cleanint", but that's not a standard command.
  It's not in my path...
- "cd ...; ..." should be "cd ... && ...".
- s/looking/Looking/
- it would be nicer to put the echo strings in quotes.

> Index: configure.in
> @@ -1960,11 +1969,11 @@
>  	LIBGRADES="$GC_LIBGRADES"
>  fi
>  
> -if test "$enable_nogc_grades" = yes; then
> +if test "$enable_prof_grades" = yes; then
>  	# add `.prof' (--profiling) grades, if time profiling is supported,
>  	# and a `.memprof' (--memory-profiling) grade.
>  	if test $mercury_cv_profiling = yes; then
> -		if test "$enable_prof_grades" = yes; then
> +		if test "$enable_nogc_grades" = yes; then
>  			DEFAULT_GRADE_NOGC="`echo $DEFAULT_GRADE | sed 's/\.gc$//'`"
>  			LIBGRADES="$LIBGRADES $DEFAULT_GRADE.prof $DEFAULT_GRADE_NOGC.prof"
>  		else

That fragment looks like a separate bug fix that is unrelated to the
deep profiling changes.  It should be committed separately
(on both the main branch and release branches).

> Index: compiler/call_gen.m

The changes to this file are not mentioned in the log message.

> Index: compiler/deep_profiling.m
...
> +:- type deep_info --->
> +	deep_info(
> +		module_info		:: module_info,
> +		pred_proc_id		:: pred_proc_id,
> +		current_csd		:: prog_var,
> +		next_site_num		:: int,
> +		call_sites		:: list(call_site_static_data),
> +		vars			:: prog_varset,
> +		var_types		:: vartypes,
> +		proc_filename		:: string,
> +		maybe_rec_info		:: maybe(deep_profile_proc_info)
> +	).

Some comments explaining the meaning of these fields might be helpful.
Especially current_csd, next_site_num, and maybe_rec_info.

Should next_site_num have type `counter' rather than `int'?

> Index: compiler/handle_options.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/handle_options.m,v
> retrieving revision 1.109
> diff -u -b -r1.109 handle_options.m
> --- compiler/handle_options.m	2001/05/15 07:11:20	1.109
> +++ compiler/handle_options.m	2001/05/15 07:16:59
> @@ -492,8 +492,39 @@
>  		[]
>  	),
>  
> -	% Deep profiling requires `procid' stack layouts
> -	option_implies(profile_deep, procid_stack_layout, bool(yes)),
> +	% Deep profiling will eventually use `procid' stack layouts,
> +	% but for now, we use a separate copy of each MR_Proc_Id structure.
> +	% option_implies(profile_deep, procid_stack_layout, bool(yes)),
> +	globals__io_lookup_bool_option(profile_deep, ProfileDeep),
> +	globals__io_lookup_bool_option(highlevel_code, HighLevel),
> +	( { ProfileDeep = yes } ->
> +		(
> +			{ HighLevel = no },
> +			{ Target = c }
> +		->
> +			[]
> +		;
> +			usage_error(
> +				"deep profiling is supported only LLDS grades")
> +		),

The error message is not grammatically correct.
It also requires users to understand what "LLDS" grades are.
I suggest something that refers to concepts that users can
more easily grasp, e.g. option names, perhaps with the original
message as a parenthetical explanatory remark:

	`--profile-deep' is incompatible with `--high-level-code'
	(deep profiling is only supported in LLDS grades)

> Index: compiler/hlds_pred.m
...
> +:- type deep_profile_role
> +	--->	inner_proc(
> +			outer_proc	:: pred_proc_id
> +		)
> +	;	outer_proc(
> +			inner_proc	:: pred_proc_id
> +		).
> +
> +:- type deep_profile_proc_info
> +	--->	deep_profile_proc_info(
> +			role		:: deep_profile_role,
> +			visible_scc	:: list(visible_scc_data)
> +					% If the procedure is not tail
> +					% recursive, this list is empty.
> +					% Otherwise, it contains outer-inner
> +					% pairs of procedures in the visible
> +					% SCC, including this procedure and
> +					% its copy.
> +		).
> +
> +:- type visible_scc_data
> +	--->	visible_scc_data(
> +			vis_outer_proc	:: pred_proc_id,
> +			vis_inner_proc	:: pred_proc_id,
> +			rec_call_sites	:: list(int)
> +					% A list of all the call site numbers
> +					% that correspond to tail calls.
> +					% (Call sites are numbered depth-first,
> +					% left-to-right, from zero.)
> +		).

Hmm, it might be nicer to put these in a different module.
They are quite specific to deep profiling and it would be
nice to keep hlds_pred.m as uncluttered as possible.

> Index: compiler/jumpopt.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/jumpopt.m,v
> retrieving revision 1.57
> diff -u -b -r1.57 jumpopt.m
> --- compiler/jumpopt.m	2000/10/31 02:15:42	1.57
> +++ compiler/jumpopt.m	2001/05/10 18:53:38
> @@ -40,7 +40,7 @@
>  
>  :- pred jumpopt_main(list(instruction)::in, set(label)::in, trace_level::in,
>  	proc_label::in, counter::in, counter::out,
> -	bool::in, bool::in, bool::in,
> +	bool::in, bool::in, bool::in, bool::in,
>  	list(instruction)::out, bool::out) is det.

The meaning of the new argument here should be documented.

> Index: compiler/layout.m
> @@ -61,6 +61,40 @@
>  			closure_file_name	:: string,
>  			closure_line_number	:: int,
>  			closure_goal_path	:: string
> +		)
> +	;	proc_static_data(
> +			proc_static_id		:: rtti_proc_label,
> +			proc_static_file_name	:: string,
> +			call_site_statics	:: list(call_site_static_data)
> +		).
> +
> +:- type call_site_static_data
> +	--->	normal_call(
> +			normal_callee		:: rtti_proc_label,
> +			normal_type_subst	:: string,
> +			normal_filename		:: string,
> +			normal_line_number	:: int,
> +			normal_goal_path	:: goal_path
> +		)
> +	;	special_call(
> +			special_filename	:: string,
> +			special_line_number	:: int,
> +			special_goal_path	:: goal_path
> +		)
> +	;	higher_order_call(
> +			higher_order_filename	:: string,
> +			ho_line_number		:: int,
> +			ho_goal_path		:: goal_path
> +		)
> +	;	method_call(
> +			method_filename		:: string,
> +			method_line_number	:: int,
> +			method_goal_path	:: goal_path
> +		)
> +	;	callback(
> +			callback_filename	:: string,
> +			callback_line_number	:: int,
> +			callback_goal_path	:: goal_path
>  		).
>  
>  :- type label_var_info
> @@ -126,7 +160,9 @@
>  	;	module_layout_string_table(module_name)
>  	;	module_layout_file_vector(module_name)
>  	;	module_layout_proc_vector(module_name)
> -	;	module_layout(module_name).
> +	;	module_layout(module_name)
> +	;	proc_static(rtti_proc_label)
> +	;	proc_static_call_sites(rtti_proc_label).

It would be helpful to have some comments here,
e.g. at least saying that these new things are for deep profiling.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
                                    |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------