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

[Fergus Henderson]

On 10-Jul-2001, Peter Ross <peter.ross at miscrit.be> wrote:
> 
> +++ compiler/ml_elim_nested.m	10 Jul 2001 14:45:39 -0000
> @@ -373,13 +373,22 @@
>  	EnvTypeEntityName = type(EnvClassName, 0),
>  	EnvTypeFlags = env_type_decl_flags,
>  	Fields = list__map(convert_local_to_field, LocalVars),
> +
> +		% IL uses classes instead of structs, so the code
> +		% generated needs to be a little different.
> +		% XXX Perhaps if we used value classes this could go
> +		% away.
> +	globals__get_target(Globals, Target),
>  	( Target = il ->
> +			% Generate a ctor for the class which
> +			% initilaises the commit field.

s/initilaises/initializes/

I don't understand what you mean by "the commit field".
*What* commit field?  Why should there be any commit field???

Oh, I had a look at the code that mlds_to_il generates for commits.
Now I finally understand what you're trying to do.
But unless I'm wildly mistaken, the code you have here is quite wrong.

For some odd reason the code in mlds_to_il uses the "Ref" argument to
do_commit as the value to throw.  This is both wierd and inefficient.
I don't know why it is done that way; maybe it is because the person
who wrote it didn't understand what the "Ref" argument was intended for,
and didn't read or didn't understand the documentation in ml_code_gen.m
about how to map do_commit/try_commit to exception handling.
The Ref argument is supposed to hold information about where to resume
execution, but for the .NET port the exception handling mechanism
takes care of that.  So there's really no need to use the Ref argument
at all.

Now, the problem that you're trying to solve here is that the Ref
argument is uninitialized, and hence is a null object reference
when the try_commit (i.e. "throw") instruction is executed.
Hence you try to initialize it here.  But this is wrong, because
here you are just processing the environment struct, and you don't
know whether there will be any commit refs in it.  If there are any,
you don't know how many there will be.  In general the number of
such references can be any integer from 0 upwards, but you are assuming 1.

The comment about perhaps using value classes is also wrong, I think;
I don't see how using value classes could help.  Certainly the object
being thrown couldn't be on the stack (the IL throw instruction requires
an object type, not a value type), and I don't see how making the environment
struct be a value type would help either.

A much better approach would be to not use the Ref arguments at all.
Then you don't need to initialize them, and you don't even need to declare them.

Of course you do need an object to throw.  The simplest thing would
be to allocate a new object in the do_commit instruction.  But a more
efficient way would be to have a single such object defined in the
Mercury runtime, allocated by the Mercury runtime startup code, and to
always throw that one.

Anyway, I have some comments on the code itself,
but these are pretty irrelevant since the right thing
to do is to just rip out this section of code entirely.

<start irrelevant comments>

>  		ThisPtr = self(mlds__commit_type),

That type is wrong; the "This" pointer is the environment type,
not the commit field inside it.

>  		FieldType = mlds__commit_type,

That looks OK.

>  		CtorType = mlds__commit_type,

CtorType is wrong here, that should be the environment struct type,
not the type of the commit field.

> +		PtrType = EnvTypeName,	
> +			
> +			% Note we have to do the correct name mangling
> +			% for the IL backend.
>  		FieldName = qual(mlds__append_name(ModuleName,
>  				EnvClassName ++ "_0"), "commit_1"),

The hard-coded _1 here is wrong.
There should be a comment explaining what the "_0" is for.

<end irrelevant comments>

>  		Lval = field(no, ThisPtr, named_field(FieldName, CtorType),
> @@ -418,11 +427,7 @@
>  	% initialize the `env_ptr' with the address of `env'
>  	%
>  	EnvVar = qual(ModuleName, mlds__var_name("env", no)),
> -	globals__get_target(Globals, Target),
> -		% IL uses classes instead of structs, so the code
> -		% generated needs to be a little different.
> -		% XXX Perhaps if we used value classes this could go
> -		% away.
> +
>  	( Target = il ->

That comment should not be removed.

>  		EnvVarAddr = lval(var(EnvVar, EnvTypeName)),
>  		ml_init_env(EnvTypeName, EnvVarAddr, Context, ModuleName,
> @@ -572,6 +577,9 @@
>  	% type declaration.
>  :- func env_type_decl_flags = mlds__decl_flags.
>  env_type_decl_flags = MLDS_DeclFlags :-
> +		% On the IL backend we use classes instead of structs so
> +		% these fields must be accessible to the mercury_code
> +		% class in the same assembly, hence the public access.
>  	Access = public,

Well, it's good that this is documented, but this is still not the right
approach.  Your change to make the access public here has broken separate
compilation for the C back-end.

One possible solution would be to make the access "default",
and then have the IL back-end map that to "public" if "assembly"
access isn't supported for top-level classes.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------