[m-rev.] diff: improve boxing and unboxing in IL backend.

[Fergus Henderson]

On 26-Jul-2001, Tyson Dowd <trd at cs.mu.OZ.AU> wrote:
> Simplify boxing, unboxing and casts in the IL backend.
> 
> compiler/mlds_to_il.m:
> 	Previously we generated calls to out-of-line procedures written
> 	in C++ (and then they forwarded to ones written in IL) for
> 	various reasons mostly to do with churn of the .NET
> 	implementation of boxing.
> 
> 	Now we generate much better code:
> 		- "unbox" and "ldobj" for unboxing
> 		- "box" for boxing
> 		- "castclass" for casts between reference types
> 		- nothing for casts to the same type
> 		- for casting from mercury types to value types we
> 		  unload from the MR_Word, and unbox the value

That's wrong for the --high-level-data case.

> Index: compiler/mlds_to_il.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/mlds_to_il.m,v
> retrieving revision 1.55
> diff -u -r1.55 mlds_to_il.m
> --- compiler/mlds_to_il.m	2001/07/20 14:14:02	1.55
> +++ compiler/mlds_to_il.m	2001/07/26 10:00:15
> @@ -1966,53 +1966,82 @@
>  
>  		% if we are casting from an unboxed type, we should box
>  		% it first.
> -		% XXX should also test the cast-to type, to handle the
> -		% cases where it is unboxed.
>  unaryop_to_il(cast(Type), Rval, Instrs) -->
>  	DataRep =^ il_data_rep,
>  	{ ILType = mlds_type_to_ilds_type(DataRep, Type) },
> -	{ 
> -		Rval = const(Const),
> -		RvalType = rval_const_to_type(Const),
> -		RvalILType = mlds_type_to_ilds_type(DataRep, RvalType),
> -		not already_boxed(RvalILType)
> -	->
> -		Instrs = node([call(convert_to_object(RvalILType)),
> -			castclass(ILType)])
> +	{ rval_to_type(Rval, RvalType) },

That change is very dangerous, since rval_to_type/2 is basically broken.
It only handles a subset of all rvals.  For the others it calls error/1.

Previously, the code here only called rval_const_to_type/1, which is safe.

Your change introduces a latent bug.  I don't know off-hand if the bug
will be actually exercised in the current compiler, but even if not,
it may be triggered later by e.g. bug fixes to the MLDS code generator
that cause it to generate more casts.

> +		( already_boxed(RvalILType) ->
> +			( RvalType = mercury_type(_, user_type) ->
> +				% XXX we should look into a nicer way to
> +				% generate MLDS so we don't need to do this
> +				Instrs = tree__list([
> +					comment_node(
> +						"loading out of an MR_Word"),
> +					instr_node(ldc(int32, i(0))),
> +					instr_node(ldelem(
> +						il_generic_simple_type)),
> +					comment_node(
> +						"turning a cast into an unbox"),
> +					convert_from_object(ILType)
> +				])

I'm pretty sure that code is wrong for --high-level-data.

> +unaryop_to_il(box(UnboxedType), _, Instrs) -->
>  	DataRep =^ il_data_rep,
> -	{ ILType = mlds_type_to_ilds_type(DataRep, Type) },
> -	{ already_boxed(ILType) ->
> -		Instrs = node([isinst(il_generic_type)])
> +	{ UnboxedILType = mlds_type_to_ilds_type(DataRep, UnboxedType) },
> +	{ already_boxed(UnboxedILType) ->
> +			% It is already boxed, so we don't need
> +			% to do anything.
> +			% It would be good if we didn't generate 
> +			% such code, but it's no big deal
> +		Instrs = empty

What do you mean by the last sentence in the comment?

If you mean that we shouldn't represent conversions like this using the
MLDS box/1 rval, then I disagree.  All type conversions in the MLDS should
be explicit, IMHO, (it would not be a good idea to allow implicit upcasts
in the MLDS) so we need some way of representing such conversions.
And that is what the MLDS box/1 rval is for: representing conversions
to mlds__generic_type.

Maybe the problem is that the box/1 and unbox/1 rvals weren't documented
well.  The names suggest that they always allocate a new value on the
heap, but that is not the case.  I've just committed a patch to mlds.m
which improves the documentation for those.  I used the names box/1 and unbox/1
since they were concise and conveyed the general intention, but maybe they
are a little misleading -- suggestions for alternative names would be welcome.

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