[m-dev.] for review: bootstrap hlc.gc.memprof grade

[Peter Ross]

On Tue, Aug 01, 2000 at 10:55:53PM +1000, Fergus Henderson wrote:
> On 01-Aug-2000, Peter Ross <peter.ross at miscrit.be> wrote:
> > compiler/mlds_to_c.m:
> >     #include mercury_imp.h at the start of the src module so that all
> >     the definitions for profiling are available.
> 
> For the MLDS back-end, I have been trying to ensure that we don't
> need to #include "mercury_imp.h", since that drags in a lot of
> stuff that is only needed for the LLDS back-end.  Instead we
> #include "mercury.h".  If additional header files are needed,
> then for consistency with the way the other ones are handled,
> they should be included from "mercury.h".  Also the #include
> should be conditional on profiling being enabled.
> 
> >     Define a new predicate mlds_output_init_fn which outputs the
> >     initialisation function.  This body of the initialisation function
> >     consists of calls to init_entry for each function in the src
> >     module.
> >     At the site of each function call: call PROFILE(callee, caller) to
> >     record the arc.
> 
> That should be `MR_PROFILE', or `MR_prof_call_profile', rather than `PROFILE'.
> (Currently the code generated by the MLDS back-end is, as far as I am
> aware, namespace clean; please keep it that way.)
> 
Ok.

> > util/mkinit.c:
> >     Call do_init_modules() if MR_HIGHLEVEL_CODE is defined.
> 
> Why?  That should not happen for grade hlc.gc.
> 
Further investigation shows that turning profiling on causes
do_init_modules() to be called at some point.  I have completely removed
all this code.


> > Index: compiler/ml_code_gen.m
> > ===================================================================
> > RCS file: /home/mercury1/repository/mercury/compiler/ml_code_gen.m,v
> > retrieving revision 1.55
> > diff -u -r1.55 ml_code_gen.m
> > --- compiler/ml_code_gen.m	2000/07/20 11:24:07	1.55
> > +++ compiler/ml_code_gen.m	2000/08/01 10:43:26
> > @@ -1670,10 +1670,16 @@
> >  		ObtainLock, ReleaseLock),
> >  
> >  	%
> > +	% Generate the MR_PROC_LABEL #define
> > +	%
> > +	ml_gen_hash_define_mr_proc_label(PredId, ProcId, HashDefine),
> > +
> > +	%
> >  	% Put it all together
> >  	%
> >  	{ Starting_C_Code = list__condense([
> >  			[raw_target_code("{\n")],
> > +			HashDefine,
> >  			ArgDeclsList,
> >  			[raw_target_code("\tstruct {\n"),
> >  			user_target_code(LocalVarsDecls, LocalVarsContext),
> > @@ -1693,6 +1699,7 @@
> >  			raw_target_code("\t\t{\n"),
> >  			user_target_code(SharedCode, SharedContext),
> >  			raw_target_code("\n\t\t;}\n"),
> > +			raw_target_code("#undef MR_PROC_LABEL\n"),
> >  			raw_target_code(ReleaseLock),
> >  			raw_target_code("\t\tif (MR_succeeded) {\n")],
> >  			AssignOutputsList
> 
> Those new lines should be added to the comment above this code,
> which says what code this code generates.
> 
> > @@ -1782,7 +1789,7 @@
> >  	% Java.
> >  	%
> >  ml_gen_ordinary_pragma_c_code(CodeModel, Attributes,
> > -		PredId, _ProcId, ArgVars, ArgDatas, OrigArgTypes,
> > +		PredId, ProcId, ArgVars, ArgDatas, OrigArgTypes,
> >  		C_Code, Context, MLDS_Decls, MLDS_Statements) -->
> >  	%
> >  	% Combine all the information about the each arg
> > @@ -1814,11 +1821,17 @@
> >  		ObtainLock, ReleaseLock),
> >  
> >  	%
> > +	% Generate the MR_PROC_LABEL #define
> > +	%
> > +	ml_gen_hash_define_mr_proc_label(PredId, ProcId, HashDefine),
> > +
> > +	%
> >  	% Put it all together
> >  	%
> >  	( { CodeModel = model_det } ->
> >  		{ Starting_C_Code = list__condense([
> >  			[raw_target_code("{\n")],
> > +			HashDefine,
> >  			ArgDeclsList,
> >  			[raw_target_code("\n")],
> >  			AssignInputsList,
> > @@ -1826,6 +1839,7 @@
> >  			raw_target_code("\t\t{\n"),
> >  			user_target_code(C_Code, yes(Context)),
> >  			raw_target_code("\n\t\t;}\n"),
> > +			raw_target_code("#undef MR_PROC_LABEL\n"),
> >  			raw_target_code(ReleaseLock)],
> >  			AssignOutputsList
> >  		]) },
> > @@ -1834,6 +1848,7 @@
> >  		ml_success_lval(SucceededLval),
> >  		{ Starting_C_Code = list__condense([
> >  			[raw_target_code("{\n")],
> > +			HashDefine,
> >  			ArgDeclsList,
> >  			[raw_target_code("\tbool SUCCESS_INDICATOR;\n"),
> >  			raw_target_code("\n")],
> > @@ -1842,6 +1857,7 @@
> >  			raw_target_code("\t\t{\n"),
> >  			user_target_code(C_Code, yes(Context)),
> >  			raw_target_code("\n\t\t;}\n"),
> > +			raw_target_code("#undef MR_PROC_LABEL\n"),
> >  			raw_target_code(ReleaseLock),
> >  			raw_target_code("\tif (SUCCESS_INDICATOR) {\n")],
> >  			AssignOutputsList
> 
> Likewise here.
> 
Done.

> > Index: compiler/mlds_to_c.m
> > ===================================================================
> > RCS file: /home/mercury1/repository/mercury/compiler/mlds_to_c.m,v
> > retrieving revision 1.46
> > diff -u -r1.46 mlds_to_c.m
> > --- compiler/mlds_to_c.m	2000/08/01 09:04:18	1.46
> > +++ compiler/mlds_to_c.m	2000/08/01 10:43:38
> > @@ -240,6 +240,7 @@
> >  
> >  	mlds_output_c_defns(MLDS_ModuleName, Indent, ForeignCode), io__nl,
> >  	mlds_output_defns(Indent, MLDS_ModuleName, NonTypeDefns), io__nl,
> > +	mlds_output_init_fn(MLDS_ModuleName, NonTypeDefns), io__nl,
> >  	mlds_output_src_end(Indent, ModuleName).
> 
> The init function should only be output if needed,
> e.g. if profiling is enabled.
> 
Done.

> >  :- pred mlds_output_hdr_start(indent, mercury_module_name,
> > @@ -280,6 +281,7 @@
> >  	io__write_string(". */\n"),
> >  	mlds_indent(Indent),
> >  	io__write_string("/* :- implementation. */\n"),
> > +	io__write_string("#include ""mercury_imp.h""\n"),
> 
> See my comments at the start.
> 
Done, but not in the relative diff.

> > +	%
> > +	% Output the function `mercury__<modulename>_init()'.
> > +	% The body of the function consists of calls
> > +	% init_entry(<function>) for each function defined in the
> > +	% module.
> 
> s/init_entry/MR_init_entry/
> 
Done.

> > +:- pred mlds_output_init_fn(mlds_module_name::in, mlds__defns::in,
> > +		io__state::di, io__state::uo) is det.
> > +
> > +mlds_output_init_fn(ModuleName, Defns) -->
> > +		% Here we ensure that we only get on "mercury__" at the
> > +		% start of the function name.
> 
> s/on/one/ ?
> 
Yes, done.

> > +		% Function prototype.
> > +	output_init_fn_name(ModuleNameString),
> > +	io__write_string(";\n"),
> > +
> > +		% Function body.
> > +	output_init_fn_name(ModuleNameString),
> > +	io__write_string("\n{\n"),
> 
> The function prototype should go in the header file, not in the .c file.
> 
Done.

> > +	io__write_strings(["\tstatic int initialised=0;\n",
> > +			"\tif (initialised) return;\n",
> > +			"\tinitialised=1;\n\n"]),
> 
> s/=0/ = 0/
> s/=1/ = 1/
> 
> > +	mlds_output_init_fn_2(ModuleName, Defns),
> > +	io__write_string("\n}\n").
> > +
> > +:- pred output_init_fn_name(string::in, io__state::di, io__state::uo) is det.
> > +
> > +output_init_fn_name(Name) -->
> > +	io__write_string("void "),
> > +	io__write_string(Name),
> > +	io__write_string("__mlds_output_init_fn(void)").
> 
> Why `__mlds_output_init_fn'?
> 
:%s problem, it has been changed back to "_init(void)" 

> > +:- pred mlds_output_init_fn_2(mlds_module_name::in, mlds__defns::in,
> > +		io__state::di, io__state::uo) is det.
> > +
> > +mlds_output_init_fn_2(_ModuleName, []) --> [].
> > +mlds_output_init_fn_2(ModuleName, [Defn | Defns]) --> 
> > +	{ Defn = mlds__defn(EntityName, _Context, _Flags, _EntityDefn) },
> > +	(
> > +		{ EntityName = function(_, _, _, _) }
> > +	->
> > +		{ QualName = qual(ModuleName, EntityName) },
> > +		io__write_string("\tinit_entry("),
> 
> s/init_entry/MR_init_entry/
> 
Done.

> > @@ -1856,6 +1922,14 @@
> >  				io__write_string("return ")
> >  			;
> >  				io__write_string("{\n"),
> > +				mlds_indent(Context, Indent + 1),
> > +
> > +				io__write_string("PROFILE("),
> > +				mlds_output_bracketed_rval(FuncRval),
> > +				io__write_string(", "),
> > +				mlds_output_fully_qualified_name(Name),
> > +				io__write_string(");\n"),
> 
> That code should only be output if profiling is enabled.
> 
> Note that for the LLDS back-end, we deliberately used macros
> and conditional compilation, but for the MLDS back-end, readability
> of the generated C code is an important goal, so we shouldn't output
> any unnecessary code, even if that code would macro-expand out to nothing.
> 
> Also it should be MR_PROFILE rather than PROFILE.
> Or you could just use MR_prof_call_profile(), which is what PROFILE()
> expands to when profiling is enabled.
> 
Done.

> > -mlds_output_atomic_stmt(Indent, NewObject, Context) -->
> > +mlds_output_atomic_stmt(Indent, FuncInfo, NewObject, Context) -->
> >  	{ NewObject = new_object(Target, MaybeTag, Type, MaybeSize,
> >  		MaybeCtorName, Args, ArgTypes) },
> >  	mlds_indent(Indent),
> >  	io__write_string("{\n"),
> >  	mlds_indent(Context, Indent + 1),
> > +
> > +	{ FuncInfo = func_info(FuncName, _FuncParams) },
> > +	io__write_string("MR_maybe_record_allocation("),
> > +	io__write_int(list__length(Args)),
> > +	io__write_string(", "),
> > +	mlds_output_fully_qualified_name(FuncName),
> > +	io__write_string(", "),
> > +	( { MaybeCtorName = yes(CtorNameA) } ->
> > +		io__write_char('"'),
> > +		c_util__output_quoted_string(CtorNameA),
> > +		io__write_char('"')
> > +	;
> > +		io__write_string("NULL")
> > +	),
> > +	io__write_string(");\n"),
> 
> That should only be output if heap profiling is enabled.
> (And then you can call MR_record_allocation() rather than
> MR_maybe_record_allocation().)
> 
Done.

> Apart from the issues mentioned above, this change looks good.  I'd
> like to see another diff when you've addressed those review comments.
> 
> Cheers,
> 	Fergus.
> 
> -- 
> 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
> --------------------------------------------------------------------------
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------