[m-rev.] for review: automatically generate all type_ctor_infos on non C backends

[Peter Ross]

On Thu, Nov 20, 2003 at 01:40:44PM +1100, Fergus Henderson wrote:
> On 19-Nov-2003, Peter Ross <pro at missioncriticalit.com> wrote:
> > Get the compiler to generate the type_ctor_infos for all the types
> > which were previously defined by hand.
> > 
> > This change completes the removal of all the MC++ from the compiler.
> 
> Good stuff, Pete.  Thanks!
> 
> > Index: compiler/special_pred.m
> ...
> >  can_generate_special_pred_clauses_for_type(ModuleInfo, TypeCtor, Body) :-
> > -	Body \= abstract_type(_),
> > +	(
> > +		Body \= abstract_type(_)
> > +	;
> > +		% Only the types which have a unification defined in
> > +		% private_builtin.m
> > +		compiler_generated_rtti_for_the_builtins(ModuleInfo),
> > +		Builtin = mercury_public_builtin_module,
> > +		( TypeCtor = qualified(Builtin, "int") - 0
> > +		; TypeCtor = qualified(Builtin, "string") - 0
> > +		; TypeCtor = qualified(Builtin, "character") - 0
> > +		; TypeCtor = qualified(Builtin, "float") - 0
> > +		; TypeCtor = qualified(Builtin, "pred") - 0
> > +		)
> 
> XXX There should be a comment in private_builtin.m pointing to this code.
> XXX Treating pred/0 specially here seems odd and possibly wrong;
>     what about func/0, pred/1, etc.?
> 
pred/0 is treated specially here because it has a unification procedure
defined in private_builtin.m, while pred/1 and func/0 don't.  Maybe the
definition of private_builtin.builtin_unify_pred should mention pred/1
instead?

> > +++ compiler/type_ctor_info.m	19 Nov 2003 14:12:28 -0000
> > @@ -111,10 +111,23 @@
> >  			TypeModuleName = ModuleName,
> >  			map__lookup(TypeTable, TypeCtor, TypeDefn),
> >  			hlds_data__get_type_defn_body(TypeDefn, TypeBody),
> > -			TypeBody \= abstract_type(_),
> > -			\+ type_ctor_has_hand_defined_rtti(TypeCtor, TypeBody),
> > -			( are_equivalence_types_expanded(ModuleInfo)
> > -				=> TypeBody \= eqv_type(_) )
> > +			(
> > +				TypeBody \= abstract_type(_),
> > +				\+ type_ctor_has_hand_defined_rtti(TypeCtor,
> > +					TypeBody),
> > +				( are_equivalence_types_expanded(ModuleInfo)
> > +					=> TypeBody \= eqv_type(_) )
> > +			;
> > +				compiler_generated_rtti_for_the_builtins(
> > +					ModuleInfo),
> > +				TypeModuleName = unqualified(ModuleNameString),
> > +				( builtin_type_ctor(ModuleNameString,
> > +					TypeName, TypeArity, _)
> > +				; impl_type_ctor(ModuleNameString,
> > +					TypeName, TypeArity, _),
> > +				  TypeName \= "ticket"
> > +				)
> > +			)
> 
> Please add some comments here.
> 
> I don't understand why we need to special-case "ticket";
> that looks like it might be a bug.
> 
It is to do with the fact that ticket is defined as having an
impl_type_ctor but it's definition is

:- type ticket == c_pointer.

What I have done is change the code to the following, which means that
we only generate type_ctor_infos for the builtin types which are
abstract.

(
        TypeBody \= abstract_type(_)
->
        \+ type_ctor_has_hand_defined_rtti(TypeCtor,
                TypeBody),
        ( are_equivalence_types_expanded(ModuleInfo)
                => TypeBody \= eqv_type(_) )
;
        % type_ctor_infos need be generated for the
        % builtin types (which are declared as abstract
        % types)
        compiler_generated_rtti_for_the_builtins(
                ModuleInfo),
        TypeModuleName = unqualified(ModuleNameString),
        ( builtin_type_ctor(ModuleNameString,
                TypeName, TypeArity, _)
        ; impl_type_ctor(ModuleNameString,
                TypeName, TypeArity, _)
        )
)


> > Index: compiler/type_util.m
> ...
> > +	% Remove any of the qualifiers from the builtin types.
> > +:- func remove_qualifiers_from_the_builtin_types(type) = (type).
> 
> I suggest s/the qualifiers/the "builtin." qualifiers/
> 
> The implementation doesn't match the documentation, since
> the implementation only removes qualifiers from some of the builtin
> types, not all of them.  E.g. it doesn't remove qualifiers from
> "builtin.c_pointer".
> 
I am doing some more investigation about this point.  It appears that we
only need to remove the qualifiers for the types which appear in
private_builtin.m with a unify predicate.


> > Index: compiler/unify_proc.m
> > +generate_builtin_unify(TypeCategory, H1, H2, Context, Clauses, !Info) :-
> > +	ArgVars = [H1, H2],
> > +	( TypeCategory = int_type,
> > +		Name = "builtin_unify_int"
> > +	; TypeCategory = char_type,
> > +		Name = "builtin_unify_character"
> > +	; TypeCategory = str_type,
> > +		Name = "builtin_unify_string"
> > +	; TypeCategory = float_type,
> > +		Name = "builtin_unify_float"
> > +	; TypeCategory = higher_order_type,
> > +		Name = "builtin_unify_pred"
> > +	; TypeCategory = tuple_type,
> > +		error("generate_builtin_unify: tuple")
> > +	; TypeCategory = enum_type,
> > +		error("generate_builtin_unify: enum")
> > +	; TypeCategory = variable_type,
> > +		error("generate_builtin_unify: variable type")
> > +	; TypeCategory = type_info_type,
> > +		error("generate_builtin_unify: type_info type")
> > +	; TypeCategory = type_ctor_info_type,
> > +		error("generate_builtin_unify: type_ctor_info type")
> > +	; TypeCategory = typeclass_info_type,
> > +		error("generate_builtin_unify: typeclass_info type")
> > +	; TypeCategory = base_typeclass_info_type,
> > +		error("generate_builtin_unify: base_typeclass_info type")
> > +	; TypeCategory = void_type,
> > +		error("generate_builtin_unify: void type")
> > +	; TypeCategory = user_ctor_type,
> > +		error("generate_builtin_unify: user_ctor type")
> > +	),
> 
> s/error/unexpected(this_file, /g
> 
> There should be a comment explaining why those cases should never occur
> (or, if they can occur, the code should do something other than just
> throwing an exception).
> 

I have added the comment in both the unify and compare clauses.

    % can_generate_special_pred_clauses_for_type ensures the unexpected
    % cases can never occur.

> > Index: library/builtin.m
> ...
> > +++ library/builtin.m	19 Nov 2003 14:12:30 -0000
> > +% These abstract type delcarations are needed so that the type_ctor
> > +% is generated for these types.
> 
> s/delcarations/declarations/
> 
> Also I suggest you say "so that the compiler will generate the
> type_ctors ..." instead of "so that the type_ctor is generated".
> 
> > +:- type int.
> > +:- type string.
> > +:- type character.
> > +:- type float.
> > +:- type (pred).
> > +:- type (func).
> > +:- type void.
> > +:- type tuple.
> 
> What happened to c_pointer?
> 
It is already defined in the interface.

> Also, I didn't see any similar declarations for the types
> defined in other modules, e.g. in library/type_desc.m.
> 
They are also already defined.

> > Index: library/type_desc.m
> ...
> >  public static int MR_compare_type_info(object[] t1, object[] t2) {
> >  	object[] res = null;
> >  
> >  	mercury.type_desc.mercury_code.ML_call_rtti_compare_type_infos(
> >  		ref res, t1, t2);
> > -/*
> > -#ifdef MR_HIGHLEVEL_DATA
> > -	return res-> data_tag;
> > -#else
> > -*/
> > +// currently comparison_results are always builting using low-level data.
> 
> "builting"?
> 
Aaarrrrgggghhhh, not all of us speak english all day! ;)  A

> > library/builtin.m:
> > -		""called compare/3 for tuple type"");
> > +		""called compare/3 for `tuple' type"");
> 
> The original phrasing was correct.  "tuple" should not be quoted here.
> 
Strange, that error doesn't appear in my version.  Maybe I fixed it
after building the diff.

> > Index: library/private_builtin.m
> > @@ -389,41 +379,8 @@
> >  
> >  ").
> >  
> > -:- pragma foreign_code("MC++", "
> > -
> > -MR_DEFINE_BUILTIN_TYPE_CTOR_INFO(private_builtin, type_ctor_info, 1,
> > -	MR_TYPECTOR_REP_TYPECTORINFO) 
> > -MR_DEFINE_BUILTIN_TYPE_CTOR_INFO(private_builtin, type_info, 1,
> > -	MR_TYPECTOR_REP_TYPEINFO) 
> > -MR_DEFINE_BUILTIN_TYPE_CTOR_INFO(private_builtin, base_typeclass_info, 1,
> > -	MR_TYPECTOR_REP_BASETYPECLASSINFO) 
> > -MR_DEFINE_BUILTIN_TYPE_CTOR_INFO(private_builtin, typeclass_info, 1,
> > -	MR_TYPECTOR_REP_TYPECLASSINFO) 
> > -").
> >  :- pragma foreign_code("C#", "
> >  
> > -/* XXX these macros need to be defined in C#
> > -// MR_DEFINE_BUILTIN_TYPE_CTOR_INFO(private_builtin, type_ctor_info, 1,
> > -//	MR_TYPECTOR_REP_TYPECTORINFO) 
> > -public static object[] __type_ctor_info_type_ctor_info_1;
> > -public static object[] private_builtin__type_ctor_info_type_ctor_info_1;
> > -
> > -// MR_DEFINE_BUILTIN_TYPE_CTOR_INFO(private_builtin, type_info, 1,
> > -//	MR_TYPECTOR_REP_TYPEINFO) 
> > -public static object[] __type_ctor_info_type_info_1;
> > -public static object[] private_builtin__type_ctor_info_type_info_1;
> > -
> > -// MR_DEFINE_BUILTIN_TYPE_CTOR_INFO(private_builtin, base_typeclass_info, 1,
> > -//	MR_TYPECTOR_REP_BASETYPECLASSINFO) 
> > -public static object[] __type_ctor_info_base_typeclass_info_1;
> > -public static object[] private_builtin__type_ctor_info_base_typeclass_info_1;
> > -
> > -// MR_DEFINE_BUILTIN_TYPE_CTOR_INFO(private_builtin, typeclass_info, 1,
> > -//	MR_TYPECTOR_REP_TYPECLASSINFO) 
> > -public static object[] __type_ctor_info_typeclass_info_1;
> > -public static object[] private_builtin__type_ctor_info_typeclass_info_1;
> > -*/
> 
> Those seem to have been defined both with and without the
> "private_builtin__prefixes"; are you sure that isn't still needed?
> 
I've looked at the generated code and they are never mentioned.  I think
they maybe an artifact from the 

> What sort of testing of the il grade have you done to ensure that
> this change didn't break things?
> 
Run "hello, world".  Believe it or not a good test because of all the
stuff that goes on in the background initialising the mercury runtime.

I have also run all the tests in hard_coded/typeclasses plus some of the
tests which have the word unify in their name.


> There was quite a bit of code which was removed,
> and some of it seems to have been _incorrectly_ removed.
> For example, the code which reports the error
> "incomparable floats in compare/3" was deleted,
> and not replaced with anything.  That has to be a bug.
> 
If it is a bug then it is a bug in the implementation of
private_builtin.builtin_compare_float as this is what is directly called
as the comparison predicate for float now.


> The chance of accidentally breaking things without any problem
> showing up immediately is higher because of the fact that undefined
> symbol errors may not be reported at link time, only at run time.
> So some care will be required to ensure this doesn't happen.
> [Running PEVerify might help detect such problems... although currently
> I think that PEVerify issues a lot of spurious warnings, because
> (a) in some places the code is unverifiable, and (b) spurious flow-on
> errors following on from (a), caused by a bug in PEVerify.]
> 
I know and it sucks!  That is why I individually added each
type_ctor_info by hand and then inspected the generated code looking for
problems.

> All of the code deletions quoted below are IMHO suspect.
> 
You will notice that all the code deletions in builtin.m are for types
which have their comparison and unification predicates defined in
Mercury in private_builtin.m.

The do_* predicates are an implementation artificat of how RTTIs are
generated on the C backend.  The il version never generates references
to them.

The __Compare* and __Unify* procedures in private_builtin.m where
duplicates of special__Compare* and special__Unify* (which still exist)
and the compiler no longer generates references to these procedures.  I
think again that they were also artifacts from porting from the C
backend.

> Otherwise, this change looks fine.
> 
Thanks but I have found a problem when bootstrapping under linux in
asm_fast.gc (the problem doesn't appear in windows).  However simply
commenting out the new abstract type definitions in builtin.m stops the
bug being triggered, but of course breaks the compilation of the il
grade.

The approach I have decided to take is to comment out the offending
definitions and check it in.  Then I will fix the problem under linux as
a seperate change.


> > Index: library/private_builtin.m
> > @@ -389,41 +379,8 @@
> > -public static bool
> > -do_unify__type_ctor_info_1_0(object[] type_info, object x, object y)
> > -{
> > -	return __Unify____type_ctor_info_1_0(type_info, 
> > -		(object[]) x, (object[]) y);
> > -}
> > -
> > -public static bool
> > -do_unify__type_info_1_0(object[] type_info, object x, object y)
> > -{
> > -	return __Unify____type_info_1_0(type_info, (object[]) x, (object[]) y);
> > -}
> > -
> > -public static bool
> > -do_unify__typeclass_info_1_0(object[] type_info, object x, object y)
> > -{
> > -	return __Unify____typeclass_info_1_0(type_info, 
> > -		(object[]) x, (object[]) y);
> > -}
> > -
> > -public static bool
> > -do_unify__base_typeclass_info_1_0(object[] type_info, object x, object y)
> > -{
> > -	return __Unify____base_typeclass_info_1_0(type_info,
> > -		(object[]) x, (object[]) y);
> > -}
> > -
> > -public static void
> > -do_compare__type_ctor_info_1_0(
> > -	object[] type_info, ref object[] result, object x, object y)
> > -{
> > -	__Compare____type_ctor_info_1_0(
> > -		type_info, ref result, (object[]) x, (object[]) y);
> > -}
> > -
> > -public static void
> > -do_compare__type_info_1_0(
> > -	object[] type_info, ref object[] result, object x, object y)
> > -{
> > -	__Compare____type_info_1_0(type_info, ref result,
> > -		(object[]) x, (object[]) y);
> > -}
> > -
> > -public static void
> > -do_compare__typeclass_info_1_0(
> > -	object[] type_info, ref object[] result, object x, object y)
> > -{
> > -	__Compare____typeclass_info_1_0(type_info, ref result,
> > -		(object[]) x, (object[]) y);
> > -}
> > -
> > -public static void
> > -do_compare__base_typeclass_info_1_0(
> > -	object[] type_info, ref object[] result, object x, object y)
> > -{
> > -	__Compare____base_typeclass_info_1_0(type_info, ref result,
> > -		(object[]) x, (object[]) y);
> > -}
> > -
> >  ").
> >  
> >  :- pragma foreign_proc("C",
> > -
> ...
> >  public static bool
> > -do_unify__heap_pointer_0_0(object x, object y)
> > -{
> > -	mercury.runtime.Errors.fatal_error(
> > -		""called unify for type `private_builtin:heap_pointer'"");
> > -	return false;
> > -}
> > -
> > -public static void
> > -do_compare__heap_pointer_0_0(
> > -	ref object[] result, object x, object y)
> > -{
> > -	mercury.runtime.Errors.fatal_error(
> > -		""called compare/3 for type `private_builtin:heap_pointer'"");
> > -}
> ...
> > -public static bool
> > -do_unify__ref_1_0(object[] type_info, object x, object y)
> > -{
> > -	return x == y;
> > -}
> > -
> ...
> > -public static void
> > -do_compare__ref_1_0(
> > -	object[] type_info, ref object[] result, object x, object y)
> > -{
> > -	mercury.runtime.Errors.fatal_error(
> > -		""called compare/3 for type `private_builtin.ref'"");
> > -}
> 
> > Index: library/type_desc.m
> ...
> >  public static void
> > -__Compare____type_ctor_desc_0_0(
> > -	ref object[] result, object[] x, object[] y)
> > -{
> > -	mercury.runtime.Errors.SORRY(
> > -		""foreign code for comparing type_ctor_descs"");
> > -}
> > -
> > -public static bool
> > -__Unify____type_ctor_desc_0_0(object[] x, object[] y)
> > -{
> > -	mercury.runtime.Errors.SORRY(
> > -		""foreign code for unifying type_ctor_descs"");
> > -	return false;
> > -}
> > -
> ...
> > -public static bool
> > -do_unify__type_ctor_desc_0_0(object x, object y)
> > -{
> > -	return __Unify____type_ctor_desc_0_0((object[]) x, (object[]) y);
> > -}
> > -
> > -public static void
> > -do_compare__type_ctor_desc_0_0(ref object[] result, object x, object y)
> > -{
> > -	__Compare____type_ctor_desc_0_0(ref result, (object[]) x, (object[]) y);
> > -}
> > -
> > -public static void
> > -__Compare____type_desc_0_0(ref object[] result, object[] x, object[] y)
> > -{
> > -	mercury.type_desc.mercury_code.ML_call_rtti_compare_type_infos(
> > -		ref result, x, y);
> > -}
> > -
> > -public static bool
> > -__Unify____type_desc_0_0(object[] x, object[] y)
> > -{
> > -	return (MR_compare_type_info(x, y) == 0);
> > -}
> > -
> ...
> > -}
> > -
> > -public static bool
> > -do_unify__type_desc_0_0(object x, object y)
> > -{
> > -	return __Unify____type_desc_0_0((object[]) x, (object[]) y);
> > -}
> > -
> > -public static void
> > -do_compare__type_desc_0_0(
> > -	ref object[] result, object x, object y)
> > -{
> > -	__Compare____type_desc_0_0(ref result, (object[]) x, (object[]) y);
> >  }
> 
> -- 
> 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
> --------------------------------------------------------------------------

-- 
Peter Ross		
Software Engineer                                (Work)   +32 2 757 10 15
Mission Critical                                 (Mobile) +32 485 482 559
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------