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

[Fergus Henderson]

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

> util/mkinit.c:
>     Call do_init_modules() if MR_HIGHLEVEL_CODE is defined.

Why?  That should not happen for grade hlc.gc.

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

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

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

> +	%
> +	% 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/

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

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

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

> +:- 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/

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

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

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