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

[Peter Ross]

Fergus wrote:
> 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.
>
Tyson will fix this up.  I have to admit that I didn't understand how the
nondet code mechanism worked, but it appears that we are getting closer to
doing it properly.

> > @@ -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.
>
That sounds like a reasonable approach.  I will fix it.

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