[m-rev.] for review: refactor mlds_to_il.m

[Peter Ross]

On Tue, Jul 10, 2001 at 12:54:34AM +1000, Tyson Dowd wrote:
> > compiler/mlds.m:
> >     Add a new field to mlds__class_defn which is the list of
> >     defns which are constructors for this class.
> >     Add the functions mlds__append_mercury_code and mlds__append_name
> >     which append either "mercury_code" or an arbitary string to the
> >     module qualifier of a name.
> 
> These new functions should almost certainly go in ml_util.m
> 
Unfortunately they can't because they need to inspect the representation
of mlds_module_name which is only available in mlds.m

I have added a comment about what these functions do to mlds.m

> > Index: ml_elim_nested.m
> > ===================================================================
> > RCS file: /home/mercury1/repository/mercury/compiler/ml_elim_nested.m,v
> > retrieving revision 1.29
> > diff -u -r1.29 ml_elim_nested.m
> > --- ml_elim_nested.m	27 Jun 2001 13:41:30 -0000	1.29
> > +++ ml_elim_nested.m	9 Jul 2001 13:08:58 -0000
> > @@ -373,8 +373,30 @@
> >  	EnvTypeEntityName = type(EnvClassName, 0),
> >  	EnvTypeFlags = env_type_decl_flags,
> >  	Fields = list__map(convert_local_to_field, LocalVars),
> > +	( Target = il ->
> > +		ThisPtr = self(mlds__commit_type),
> > +		FieldType = mlds__commit_type,
> > +		CtorType = mlds__commit_type,
> > +		PtrType = mlds__commit_type,	% XXX
> > +		FieldName = qual(mlds__append_name(ModuleName,
> > +				EnvClassName ++ "_0"), "commit_1"),
> 
> The XXX isn't explained.
> 
I wasn't sure what to set it as but it appears that the IL backend never
looks at this field so it doesn't matter.  Maybe we should add an
invalid type for these sorts of situations.

> > @@ -548,7 +570,7 @@
> >  	% type declaration.
> >  :- func env_type_decl_flags = mlds__decl_flags.
> >  env_type_decl_flags = MLDS_DeclFlags :-
> > -	Access = private,
> > +	Access = public,
> >  	PerInstance = one_copy,
> >  	Virtuality = non_virtual,
> >  	Finality = overridable,
> 
> I'm not 100% clear on why this change needs to be made.  An explanation
> would be good.
> 
The following except from the log comment explains it.

    On the IL backend the mercury code is placed in a seperate class to
    the environment data, so the env_type decls must be public so as to
    be accessible from the code.

If the above is not understandable here is the code that we generate.

.class mercury_code {
    .method f(...)
    {
        ldfld env::commit   // reference to field in another class.
    }
}

.class env {
    .field  commit  // Must be public so that the field is accesible for
                    // the mercury_code class.
}


> > -
> >  %-----------------------------------------------------------------------------%
> > +%-----------------------------------------------------------------------------%
> > +
> > +generate_il(MLDS, AssemblyDecls ++ [assembly(IlInfo ^ assembly_name),
> > +		namespace(NamespaceName, ILDecls)], set__init, IO0, IO) :-
> > +	transform_mlds(MLDS) = mlds(MercuryModuleName, _, Imports, Defns),
> >  
> 
> I'd prefer to have the LHS contain the output of the function, but
> maybe this is just my inflexible brain...
> 
> > +	% Move all the top level methods and data definitions into the
> > +	% mercury_code class, and then rename all the definitions as
> > +	% necessary to reflect this new hierachy.
> > +:- func transform_mlds(mlds) = mlds.
> > +
> > +transform_mlds(mlds(MercuryModuleName, ForeignCode, Imports, Defns))
> > +	= mlds(
> > +		MercuryModuleName,
> > +		ForeignCode,
> > +		Imports,
> > +		[mercury_code_class(list__map(rename_defn, Members)) | Others]
> > +	) :-
> > +	list__filter((pred(D::in) is semidet :-
> > +			( D = mlds__defn(_, _, _, mlds__function(_, _, _))
> > +				% XXX we need to place all the RTTI
> > +				% datastructures inside this class, so
> > +				% they are generated as fields.
> > +				% Maybe what we should do is make all
> > +				% the RTTI data structures nested
> > +				% classes.  I think that would work
> > +				% better.
> > +			; D = mlds__defn(_, _, _, mlds__data(_, _))
> 
> Agreed, this is a nice idea, but it's a bit more work.
> I would possibly reduce startup time, but increase the number of
> classes.  Worth investigating in future.
> 
> > +:- func rename_defn(mlds__defn) = mlds__defn.
> > +
> > +rename_defn(defn(Name, Context, Flags, Entity0))
> > +	= defn(Name, Context, Flags, Entity) :-
> > +	( Entity0 = data(Type, Initializer),
> > +		Entity = data(Type, rename_initializer(Initializer))
> > +	; Entity0 = function(MaybePredProcId, Params, MaybeStmt0),
> > +		( MaybeStmt0 = yes(Stmt),
> > +			MaybeStmt = yes(rename_statement(Stmt))
> > +		; MaybeStmt0 = no,
> > +			MaybeStmt = no
> >  		),
> > +		Entity = function(MaybePredProcId, Params, MaybeStmt)
> > +	; Entity0 = class(_),
> > +		sorry(this_file, "renaming nested classes")
> > +	).
> 
> We're going to need nested classes, they are generated in --grade il.
> (high-level data).
> 
Ok, I will do these as a seperate change immediately.  As this change
will require that I refactor the code some more.

> > +
> > +:- func rename_statement(mlds__statement) = mlds__statement.
> > +
> > +rename_statement(statement(block(Defns, Stmts), Context))
> > +	= statement(block(list__map(rename_defn, Defns),
> > +			list__map(rename_statement, Stmts)), Context).
> > +rename_statement(statement(while(Rval, Loop, IterateOnce), Context))
> > +	= statement(while(rename_rval(Rval),
> > +			rename_statement(Loop), IterateOnce), Context).
> > +rename_statement(statement(if_then_else(Rval, Then, MaybeElse0), Context))
> > +	= statement(if_then_else(rename_rval(Rval),
> > +			rename_statement(Then), MaybeElse), Context) :-
> > +	( MaybeElse0 = no,
> > +		MaybeElse = no
> > +	; MaybeElse0 = yes(Else),
> > +		MaybeElse = yes(rename_statement(Else))
> > +	).
> > +rename_statement(statement(switch(_, _, _, _, _), _Context))
> > +	= _ :- sorry(this_file, "rename switch").
> 
> This isn't good, we should process switches, because they are used for
> string switches, which Fergus has recently made work.
> 
Implemented, I just hadn't gotten around to doing yet.

> > +:- func mlds_defn_to_ilasm_decl(il_info, mlds__defn) = ilasm__decl.
> > +
> > +mlds_defn_to_ilasm_decl(_, defn(_Name, _Context, _Flags, data(_Type, _Init)))
> > +	= _ :- sorry(this_file, "top level data definition!").
> > +mlds_defn_to_ilasm_decl(_, defn(_Name, _Context, _Flags,
> > +		function(_MaybePredProcId, _Params, _MaybeStmts)))
> > +	= _ :- sorry(this_file, "top level function definition!").
> > +mlds_defn_to_ilasm_decl(Info0, defn(Name, _Context, Flags, class(ClassDefn)))
> > +	= class(
> > +		decl_flags_to_classattrs(Flags),
> > +		EntityName,
> > +		Extends,
> > +		Interfaces,
> > +		MethodDecls
> > +	) :-
> > +	EntityName = entity_name_to_ilds_id(Name),
> > +	ClassDefn = class_defn(_Kind, _Imports, Inherits, Implements,
> > +			Ctors, Members),
> > +	( Inherits = [],
> > +		Extends = extends_nothing,
> > +		Parent = structured_name("mscorlib", ["System", "Object"])
> 
> It's preferable to use 
> 		Parent = il_generic_class_name
> for this.
> 
Thanks for some reasone I couldn't find this function.  I did look for
it.

> > -			% Retrieve the locals, put them in the enclosing
> > -			% scope.
> > -		il_info_get_locals_list(Locals),
> > -		{ InstrsTree = tree__list([
> > -			context_node(Context),
> > -			instr_node(start_block(scope(Locals), BlockId)),
> > -			InstrsTree1, 
> > -			MaybeRet,
> > -			instr_node(end_block(scope(Locals), BlockId))
> > -			])
> > -		},
> > +	Interfaces = implements(
> > +			list__map(interface_id_to_class_name, Implements)),
> > +
> > +	ClassName = class_name(Info0 ^ module_name, EntityName),
> > +	list__map_foldl(generate_method(ClassName, no), Members,
> > +			MethodsAndFields, Info0, Info1),
> > +	list__map_foldl(generate_method(ClassName, yes(Parent)), Ctors,
> > +			IlCtors, Info1, Info),
> > +	MethodsAndFieldsAndCtors = IlCtors ++ MethodsAndFields,
> > +
> > +		% XXX Maybe it would be better to just check to see
> > +		% whether or not there are any init instructions then
> > +		% explicitly checking for the name mercury_code.
> 
> s/then/than
> 
> And yes, this would be nicer I think, but you don't have to do it now.
> 
> > +:- func decl_flags_to_methattrs(mlds__decl_flags) = list(ilasm__methattr).
> > +
> > +decl_flags_to_methattrs(Flags)
> > +	= list__condense([Access, PerInstance, Virtuality,
> > +			Finality, Abstractness]) :-
> > +	AccessFlag = access(Flags),
> > +	( AccessFlag = public,
> > +		Access = [public]
> > +	; AccessFlag = protected,
> > +			% XXX is this correct?
> > +		Access = [family]
> 
> Family seems to be right.
> 
> > -generate_method_defn(DataDefn) --> 
> > -	{ DataDefn = defn(data(DataName), Context, _DeclsFlags, Entity) },
> >  	il_info_get_module_name(ModuleName),
> > -	{ ClassName = mlds_module_name_to_class_name(ModuleName, yes) },
> >  
> > +	/*
> >  		% Generate a term (we use it to emit the complete
> >  		% method definition as a comment, which is nice
> >  		% for debugging).
> > -	{ term__type_to_term(DataDefn, MLDSDefnTerm) },
> > +	{ term__type_to_term(defn(Name, Context, Flags, Entity),
> > +			_MLDSDefnTerm) },
> > +	*/
> 
> This is commented out but not explained.
> 
To get this working would require that we change the implementation to
return a list of definitions, so that we could add the comment
definition.

> > -%-----------------------------------------------------------------------------
> > +	{ ClassDecl = ilasm__method(methodhead(Attrs, MemberName,
> > +			ILSignature, []), MethodContents)}.
> > +
> > +generate_method(_, _, defn(_Name, _Context, _Flags, Entity), _ClassDecl) -->
> > +	{ Entity = class(_ClassDefn) },
> > +	{ sorry(this_file, "nested classes") }.
> 
> Nested classes were implemented previously, you have deleted their
> implementation:
> 
> > -defn_to_class_decl(mlds__defn(EntityName, _Context, _DeclFlags,
> > -		mlds__class(ClassDefn)), ILClassDecl) -->
> > -	DataRep =^ il_data_rep,
> > -	( { EntityName = type(TypeName0, Arity) } ->
> > -		{ TypeName = string__format("%s_%d",
> > -			[s(TypeName0), i(Arity)]) },
> > -		{ ClassDefn = mlds__class_defn(_ClassType, _Imports, 
> > -			Inherits, _Implements, Defns) },
> > -		{ FullClassName = structured_name("", [TypeName]) },
> > -		list__map_foldl(defn_to_class_decl, Defns, ILDefns),
> > -		{ make_constructor(DataRep, FullClassName, ClassDefn,
> > -			ConstructorILDefn) },
> > -		{ Extends = mlds_inherits_to_ilds_inherits(DataRep, Inherits) },
> > -		{ ILClassDecl = nested_class([public], TypeName, Extends,
> > -			implements([]), [ConstructorILDefn | ILDefns]) }
> > -	;
> > -		{ error("expected type entity name for a nested class") }
> > -	).
> > -
> 
> This should *not* be removed, they are required for --grade il.
> 
> > @@ -2745,7 +2987,9 @@
> >  make_method_defn(InstrTree) = MethodDecls :-
> >  	Instrs = list__condense(tree__flatten(InstrTree)),
> >  	MethodDecls = [
> > -		maxstack(int32(calculate_max_stack(Instrs))),
> > +			% XXX The + 1 is needed for when the --debug-il-asm
> > +			% flag is enabled.
> > +		maxstack(int32(calculate_max_stack(Instrs) + 1)),
> >  			% note that we only need .zeroinit to ensure
> >  			% verifiability; for nonverifiable code,
> 
> This + 1 should be conditional on --debug-il-asm which can be looked up
> in the globals.
> 
> I'd prefer all the hacks that --debug-il-asm requires to be conditional
> so that when we get a decent .NET debugger (and therefore don't need
> --debug-il-asm anymore) we can easily remove it.
> 
Ok done.

> 
> Apart from this the change is fine.  However you definitely need to put
> back the code for nested classes.
> 
As I said I will put that back as a seperate change.

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