[m-rev.] for review: term size profiling

[Fergus Henderson]

Firstly, I want to say that it would be much, *much*, *MUCH*
easier to review this change if the change was broken into
smaller parts.  12000 lines of diff is just too much to review
at once; it's too difficult to keep enough of it in your head
at once to make sure that the change is correct, consistent,
and complete.

Here are some parts that could be split out as separate changes:

1.

> compiler/goal_util.m:
> compiler/hlds_module.m:
> 	When generating calls, specify whether the call is to a predicate or
> 	function. Previously, all calls generated here were to predicates,
> 	but size_prof wants to add calls to int.+. Actually, we call the
> 	function term_size_prof_builtin.term_size_plus, which is the same
> 	as int.+, except it doesn't get deleted by dead proc elimination.

2.

> compiler/handle_options.m:
> 	Add a -D flag value to print HLDS components relevant to HLDS
> 	transformations.
> 
> compiler/hlds_out.m:
> 	Print more details of unifications if HLDS dump flags call for it.
> 	(The flag test is in the caller of the modified predicate.)
> 
> 	Print the goal annotations that specify which variables need to be
> 	saved on the stack on one line, to allow people to grep for them.
> 
> doc/user_guide.tex:
> 	Document the extended output from specifying the the 'u' flag.

3.

> runtime/mercury_engine.[ch]:
> 	Define a new debugging flag whose purpose is to allow an mdb command,
> 	"flag enabled on", to turn on low level debugging if the
> 	executable was compiled with the appropriate options.

(and any changes elsewhere to make existing code use this new flag).

4.

> runtime/mercury_proc_id.h:
> runtime/mercury_univ.h:
> 	New header files. mercury_proc_id.h contains the (unchanged)
> 	definition of MR_Proc_Id, while mercury_univ.h contains the
> 	definitions of the macros for manipulating univs that used to be
> 	in mercury_type_info.h, updated to use the new macros for allocating
> 	memory.
> 
> 	In the absence of these header files, the following circularity
> 	would ensue:
> 
> 	mercury_deep_profiling.h includes mercury_stack_layout.h
> 		- needs definition of MR_Proc_Id
> 	mercury_stack_layout.h needs mercury_type_info.h
> 		- needs definition of MR_PseudoTypeInfo
> 	mercury_type_info.h needs mercury_heap.h
> 		- needs heap allocation macros for MR_new_univ_on_hp
> 	mercury_heap.h includes mercury_deep_profiling.h
> 		- needs MR_current_call_site_dynamic for recording allocations
> 
> 	Breaking the circular dependency in two places, not just one, is to
> 	minimize similar problems in the future.
> 
> runtime/mercury_types.h:
> 	Move more typedefs here from other header files, such as
> 	mercury_stack_layout.h and mercury_type_info.h; the declarations of the
> 	structures they refer to stay in their original files. These forward
> 	declarations reduce the need to include the header files that
> 	previously defined these type names at the tops of other header files,
> 	and thus reduce the risk of circular dependencies.
> 
> 	Move up the definition of MR_Box for use by some of these typedefs.
> 
> runtime/mercury_stack_layout.h:
> 	Delete the definition of MR_Proc_Id, which is now in mercury_proc_id.h.
> 
> 	Delete typedefs which are now in mercury_types.h.
> 
> 	Add a macro to compute the max MR_mrN register used at a label, to
> 	reduce code duplication in the trace directory.
> 
> runtime/mercury_type_info.h:
> 	Delete the macros for manipulating univs, which are now in
> 	mercury_univ.h.
> 
> 	Delete typedefs which are now in mercury_types.h.
> 
> runtime/mercury_deep_profiling.h:
> 	#include mercury_proc_id.h instead of mercury_stack_layout.h.
> 
> runtime/Mmakefile:
> 	Mention the new files.
> 
> runtime/mercury_imp.h:
> runtime/mercury.h:
> runtime/mercury_construct.c:
> 	Include the new files at appropriate points.

5.

> trace/mercury_trace_internal.c:
> 	Add an option, -i to the forward movement commands, which causes
> 	the debugger to perform an integrity check of all ancestor stack frames
> 	at every event. This is useful when looking for code generation errors
> 	that cause cells to be built wrong.
> 
> trace/mercury_trace.[ch]:
> trace/mercury_trace_vars.[ch]:
> 	Implement the integrity check.

6.

> trace/mercury_trace_vars.[ch]:
> 
> 	Decide whether to print the name of a variable before invoking the
> 	supplied print or browse predicate on it based on a flag design for
> 	this purpose, instead of overloading the meaning of the output FILE *
> 	variable. This arrangement is much clearer.

Could you please split off at least some of these and post them as
separate diffs?

But I do have some review comments just from looking at the log message
and the changes to user_guide.texi:
- There should be some test cases to test the new functionality.
- The new features should be documented in the user guide.
  Not all of them were.
- For stuff which is added to the user guide but commented out,
  there should also be a comment explaining why it is commented out.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  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
--------------------------------------------------------------------------