[m-rev.] [b.schmidt at ugrad.unimelb.edu.au: MLDS Accurate GC fixes]

[Julien Fischer]

On Thu, 14 Dec 2006, Zoltan Somogyi wrote:

> ----- Forwarded message from Ben Schmidt <b.schmidt at ugrad.unimelb.edu.au> -----
>
> Date: Thu, 14 Dec 2006 16:52:10 +1100
> From: Ben Schmidt <b.schmidt at ugrad.unimelb.edu.au>
> To: Zoltan Somogyi <zs at csse.unimelb.edu.au>
> Subject: MLDS Accurate GC fixes
>
> Proposed diff and log message attached.
>
> Ben.
>
>
> Estimated hours taken: 25 (though some were spent learning build system, etc.)
> Branches: <which CVS branches ("main", "release", etc.) this change
>           will be committed on>

This should be on just the main branch.

> Bug fixes for the MLDS accurate garbage collector.

What is the status of the hlc.agc grade with this change? 
IIRC, it previously couldn't compile library/rtti_implementation.m.

> compiler/mlds.m:
> compiler/ml_code_util.m:
> compiler/ml_elim_nested.m:
> compiler/ml_closure_gen.m
> compiler/mlds_to_c.m:
> compiler/mlds_to_il.m:
> 	Change type of mlds_maybe_gc_trace_code to a discriminated union
> 	to allow separation of initialisation and variable tracing code,
> 	rather than the standard yes/no maybe type. Update predicate/
> 	function definitions accordingly.

For most of those modules you can simply say something like:
conform to the above changes.  It's really only mlds.m that has changed
in a meaningful way.  Also the changesto ml_elim_nested.m are listed
below.

> compiler/ml_elim_nested.m:
> 	- Update extract_gc_trace_code to provide initialisation and
> 	  variable tracing code separately.
> 	- Change method of flattening so that pulling out of variables into

s/pulling out/hoisting/

> 	  structs and changing references to those structs are
> 	  sequenced, not interleaved; this fixed a bug where references to

Make this into two separate sentences.

s/This fixed/This fixes/

> 	  a variable could be processed before that variable was pulled out
> 	  into a struct, and thus the references not properly updated.

...

> compiler/ml_closure_gen.m:
> 	When generating definitions for 'closure_layout_ptr' and
> 	'type_params', use 'init' for the garbage collection code, as
> 	it is initialisation required by the tracing code for other
> 	variables rather than variable tracing code itself, and as such
> 	must occur first in the GC tracing function.


I suggest:

 	Ensure that any initialisation required by the by the GC
 	tracing code is carried out before the tracing code is
 	executed.

...

> Index: compiler/ml_elim_nested.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/ml_elim_nested.m,v
> retrieving revision 1.85
> diff -u -r1.85 ml_elim_nested.m
> --- compiler/ml_elim_nested.m	22 Aug 2006 05:03:54 -0000	1.85
> +++ compiler/ml_elim_nested.m	11 Dec 2006 23:51:12 -0000
> @@ -529,8 +529,8 @@
>         Params0 = mlds_func_params(Arguments0, RetValues),
>         ml_maybe_add_args(Arguments0, FuncBody0, ModuleName,
>             Context, ElimInfo0, ElimInfo1),
> -        flatten_arguments(Arguments0, Arguments1, ElimInfo1, ElimInfo2),
> -        flatten_statement(FuncBody0, FuncBody1, ElimInfo2, ElimInfo),
> +        flatten_statement(FuncBody0, FuncBody1, ElimInfo1, ElimInfo2),
> +        fixup_gc_trace_code(ElimInfo2, ElimInfo),
>         elim_info_finish(ElimInfo, NestedFuncs0, Locals),
>
>         % Split the locals that we need to process into local variables
> @@ -595,7 +595,7 @@
>                 % functions, or (for accurate GC) may contain pointers,
>                 % then we need to copy them to local variables in the
>                 % environment structure.
> -                ml_maybe_copy_args(Arguments1, FuncBody0, ElimInfo,
> +                ml_maybe_copy_args(Arguments0, FuncBody0, ElimInfo,
>                     EnvTypeName, EnvPtrTypeName, Context,
>                     _ArgsToCopy, CodeToCopyArgs),
>
> @@ -645,10 +645,10 @@
>             % annotation on the arguments anymore. We delete them here, because
>             % otherwise the `#if 0 ... #endif' blocks output for the
>             % annotations clutter up the generated C files.
> -            Arguments = list.map(strip_gc_trace_code, Arguments1)
> +            Arguments = list.map(strip_gc_trace_code, Arguments0)
>         ;
>             Action = hoist_nested_funcs,
> -            Arguments = Arguments1
> +            Arguments = Arguments0
>         ),
>         Params = mlds_func_params(Arguments, RetValues),
>         DefnBody = mlds_function(PredProcId, Params,
> @@ -681,7 +681,7 @@
>             [], [FuncBody])
>     ->
>         ml_conv_arg_to_var(Context, Arg, ArgToCopy),
> -        elim_info_add_and_flatten_local_data(ArgToCopy, !Info)
> +        elim_info_add_local_data(ArgToCopy, !Info)
>     ;
>         true
>     ),
> @@ -815,21 +815,23 @@
>     Fields0 = list.map(convert_local_to_field, LocalVars),
>
>     % Extract the GC tracing code from the fields.
> -    list.map2(extract_gc_trace_code, Fields0, Fields1, GC_TraceStatements0),
> -    GC_TraceStatements = list.condense(GC_TraceStatements0),
> +    list.map3(extract_gc_trace_code, Fields0, Fields1,
> +        GC_InitStatements,GC_TraceStatements),
> +    list.append(GC_InitStatements,GC_TraceStatements,GC_Statements0),
> +    GC_Statements = list.condense(GC_Statements1),
>
>     ( Action = chain_gc_stack_frames ->
> -        ml_chain_stack_frames(Fields1, GC_TraceStatements, EnvTypeName,
> +        ml_chain_stack_frames(Fields1, GC_Statements, EnvTypeName,
>             Context, FuncName, ModuleName, Globals, Fields, EnvInitializer,
>             LinkStackChain, GCTraceFuncDefns),
>         GC_TraceEnv = no
>     ;
>         (
> -            GC_TraceStatements = [],
> +            GC_Statements = [],
>             GC_TraceEnv = no
>         ;
> -            GC_TraceStatements = [_ | _],
> -            GC_TraceEnv = yes(ml_block([], GC_TraceStatements, Context))
> +            GC_Statements = [_ | _],
> +            GC_TraceEnv = yes(ml_block([], GC_Statements, Context))
>         ),
>         Fields = Fields1,
>         EnvInitializer = no_initializer,
> @@ -1034,15 +1036,21 @@
>         Virtuality, Finality, Constness, Abstractness).
>
> :- pred extract_gc_trace_code(mlds_defn::in, mlds_defn::out,
> -    statements::out) is det.
> +    statements::out,statements::out) is det.

s/out,s/out, s/

...

> @@ -1871,6 +1850,47 @@
> fixup_lval(var(Var0, VarType), VarLval, !Info) :-
>     fixup_var(Var0, VarType, VarLval, !Info).
>
> +% fixup_gc_trace_code:
> +%   Process the trace code in the locals that have been pulled out

s/pulled out/hoisted/

> +%   to the stack frame structure so that the code correctly refers to any
> +%   variables that have been pulled out.
> +%   It assumes the locals don't actually change during the process.
> +%   I think this should be safe. (schmidt)
> +
> +:- pred fixup_gc_trace_code(elim_info::in, elim_info::out) is det.
> +
> +fixup_gc_trace_code(!Info) :-
> +    Locals = elim_info_get_local_data(!.Info),
> +    fixup_gc_trace_code_defns(Locals, !Info).
> +
> +:- pred fixup_gc_trace_code_defns(mlds_defns::in,
> +    elim_info::in, elim_info::out) is det.
> +
> +fixup_gc_trace_code_defns([], !Info).
> +fixup_gc_trace_code_defns([Defn0|Defns], !Info) :-

s/[Defn0|Defns]/[ Defn0 | Defns ]/

> +    % The ordering of variables is important, and must not be disturbed,
> +    % so this ordering is important. If other parts of the compiler

The first sentence does not parse well.  I suggest:

 	The ordering of variables here is important and must be
 	maintained by subsequent passes.

> +    % end up changing the order variables are defined, this routine

the order _in which_ variables are defined

> +    % may need to change also, or flags for the important initialisations
> +    % in GC trace code (closure_layout_ptr in ml_gen_closure.m, etc.)
> +    % set so they are kept at the top.
> +    % Ordering will become unimportant due to other modifications soon ???
> +    % Wrecking the ordering for testing ???

The last two setences don't seem to serve any purpose.
If they relate to a future change then please mark them with an XXX.

> +    %fixup_gc_trace_code_defns(Defns, !Info),
> +    (
> +        Defn0 = mlds_defn(Name, Context, Flags, DefnBody0),
> +        DefnBody0 = mlds_data(Type, Init, MaybeGCTraceCode0)
> +    ->
> +        flatten_maybe_gc_trace_code(MaybeGCTraceCode0, MaybeGCTraceCode, !Info),
> +        DefnBody = mlds_data(Type, Init, MaybeGCTraceCode),
> +        Defn = mlds_defn(Name, Context, Flags, DefnBody),
> +        elim_info_remove_local_data(Defn0, !Info),
> +        elim_info_add_local_data(Defn, !Info)
> +    ;
> +        true
> +    ), %.
> +    fixup_gc_trace_code_defns(Defns, !Info).

Why are those commented-out pieces of code there?

...

> Index: compiler/mlds.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/mlds.m,v
> retrieving revision 1.140
> diff -u -r1.140 mlds.m
> --- compiler/mlds.m	22 Aug 2006 05:03:56 -0000	1.140
> +++ compiler/mlds.m	11 Dec 2006 23:51:15 -0000
> @@ -588,7 +588,16 @@
>     % `no' here indicates that no GC tracing code is needed,
>     % e.g. because accurate GC isn't enabled, or because the
>     % variable can never contain pointers to objects on the heap.
> -:- type mlds_maybe_gc_trace_code == maybe(statement).
> +    % 'yes' indicates that GC tracing code is required for the
> +    % variable.
> +    % 'init' indicates that the variable is a compiler-generated
> +    % variable that needs to be initialised before traversing the
> +    % other variables for the predicate (used when generating
> +    % closures).
> +:- type mlds_maybe_gc_trace_code
> +    ---> yes(statement)
> +    ;    init(statement)
> +    ;    no.

Rename the constructors of this type so that they are more distinctive, 
e.g.

 		% If accurate GC is enabled, we associate with each
 		% variable (including function parameters), the code
 		% needed to trace that variable when doing GC.
 		%
 	:- type mlds_gc_trace_code
 		--->	gctc_trace_code(statement)
 			% Code required to trace a variable.

 		;	gctc_initialise(statement)
 			% Code required to initialise a compiler-generated
 			% variable such as closure_layout pointer.

 		;	gctc_none.
 			% No tracing code, either because accurate GC
 			% is not enabled or the variable never contains
 			% pointers to objects on the heap.

I would remove the `maybe' from the type name since to most of the compiler
developers that would suggest that it is a maybe type which it no longer is.

The comment associated with the mlds_data field of the mlds_entity_defn type
in mlds.m about the gc trace code is no longer accurate as the code
associated with that field may now be used initialise a variable rather than
trace it.

Could you please post a revised version of this diff after you have
addressed the above comments.

Julien.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------