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

[Zoltan Somogyi]

On 17-May-2001, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> 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.

I'll see what I can do.

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

Good idea.

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

Yes.

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

They output proc_static structures; I updated the log message.

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

That's your fault; it has been in the tools directory since 1996.
I updated that to read ../tools/cleanint.

> - "cd ...; ..." should be "cd ... && ...".
> - s/looking/Looking/

Done.

> - it would be nicer to put the echo strings in quotes.

Either it goes in quotes, or it fits on one line. I prefer the latter.

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

Is version-0_10 or version-0_10_y the release branch?

> > Index: compiler/call_gen.m
> 
> The changes to this file are not mentioned in the log message.

Ah, that's because I wanted to commit that separately. The log message for
that change is:

compiler/call_gen.m:
	Optimize the handling of calls to procedures whose determinism is
	failure by omitting the check of r1 after the call that we generate
	for other model_semi procedures. Instead, the code we generate fails
	unconditionally. This optimization is useful for deep profiling,
	which inserts calls to such procedures into programs.

> > 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)
> > +	).
> 
> Should next_site_num have type `counter' rather than `int'?

Yes, I think so.

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

I'll see what I can do, but some parts really should be documented by Tom :-(

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

The problem with such a message is that both deep profiling and high level code
can be switched on in many ways: via grade components and via more than one
option (e.g. --profile-deep is mostly for internal use; it is one of several
options set by --deep-profiling).

For now, I have changed that message to

	deep profiling is incompatible with high level code

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

The problem is that these structures are full of pred_proc_ids, so that
module would have to import hlds_pred, and hlds_pred would have to import it.
I think this level of clutter is a smaller problem than a circular dependency
between hlds_pred.m and a new module, and a *much* smaller problem than
circular dependency between hlds_pred.m and deep_profiling.m (the other
natural home of these type definitions is of course deep_profiling.m).

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

Done.

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

Will do.

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