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

[Tyson Dowd]

On 09-Jul-2001, Peter Ross <peter.ross at miscrit.be> wrote:
> Hi,
> 
> For Tyson or Fergus to review.
> 
> 
> ===================================================================
> 
> 
> Estimated hours taken: 40
> Branches: main
> 
> Refactor the top level of mlds_to_il so that we only do one pass over
> the MLDS to generate the ILDS.  As a side effect of this change nondet
> code now works again.

Thanks Pete, here's your review:

> 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

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

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

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

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

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

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

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


Apart from this the change is fine.  However you definitely need to put
back the code for nested classes.

-- 
       Tyson Dowd           # 
                            #  Surreal humour isn't everyone's cup of fur.
     trd at cs.mu.oz.au        # 
http://www.cs.mu.oz.au/~trd #
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------