[m-dev.] for review: merge HAL branch onto main branch
Fergus Henderson
fjh at cs.mu.OZ.AU
Thu Feb 15 20:54:35 AEDT 2001
On 15-Feb-2001, David Jeffery <dgj at cs.mu.OZ.AU> wrote:
> compiler/type_util.m:
> Add predicates `type_util__constructors_are_dummy_argument_type' and
> `type_constructors_are_type_info'.
Please append "... for use by ..." to explain the rationale for
adding these.
> +++ compiler/make_hlds.m 2001/02/15 03:11:28
> @@ -1818,10 +1819,15 @@
> CtorFields, Module1a) },
> globals__io_lookup_bool_option(unboxed_no_tag_types,
> AllowNoTagTypes),
> -
> + globals__io_lookup_bool_option(reserve_tag,
> + ReserveTag),
> {
> - AllowNoTagTypes = yes,
> - type_constructors_are_no_tag_type(ConsList,
> + (
> + AllowNoTagTypes = yes, ReserveTag = no
> + ;
> + type_util__is_dummy_argument_type(Type)
> + ),
Why do you treat dummy argument types as no_tag types even when
AllowNoTagTypes is `no'?
That at least needs documenting.
> +++ compiler/make_tags.m 2001/02/15 03:52:52
> @@ -66,25 +66,40 @@
> % work out how many tag bits there are
> globals__lookup_int_option(Globals, num_tag_bits, NumTagBits),
>
> + % determine if we need to reserve a tag
> + % (this also disables enumerations and no_tag types)
> + globals__lookup_bool_option(Globals, reserve_tag, ReserveTag),
> + ( ReserveTag = yes, \+ type_constructors_are_type_info(Ctors) ->
> + InitTag = 1
> + ;
> + InitTag = 0
> + ),
Here, and also in handle_options.m, it would help to explain in a little bit more
detail:
s/reserve a tag/reserve a tag for <purpose>/
for some appropriate value of <purpose>.
Also you should document why the setting of the option is ignored
for type_infos.
> % assign single functor of arity one a `no_tag' tag
> - % (unless it is type_info/1)
> + % (unless it is type_info/1 or we are reserving a tag)
> globals__lookup_bool_option(Globals,
> unboxed_no_tag_types, yes),
> type_constructors_are_no_tag_type(Ctors, SingleFunc,
> - SingleArg, _)
> + SingleArg, _),
> + (
> + ReserveTag = no
> + ;
> + constructors_are_dummy_argument_type(Ctors)
> + )
> ->
The comment here doesn't match the code anymore, in particular
with regard to the call to constructors_are_dummy_argument_type,
which is not explained in the comment.
> @@ -92,6 +107,13 @@
> ;
> NumTagBits = 0
> ->
> + ( ReserveTag = yes ->
> + % XXX Need to fix this.
> + % What architectures does this occur on?
> + error("Oops: sorry, not implemented: --reserve-tag with num_tag_bits = 0")
> + ;
> + true
> + ),
NumTagBits = 0 is used for the .NET and Java ports.
Use sorry/2 from error_util.m rather than error/1.
options.m:
> @@ -2026,6 +2029,10 @@
> + "--reserve-tag\t\t\t(grade modifier: `.rt')",
> + "\tReserve a tag.",
> + "\tThis is necessary for a seamless Herbrand solver -",
> + "\tintended for use with HAL.",
The comment "Reserve a tag" doesn't help much.
Please include a much more verbose explanation.
> Index: compiler/rtti_out.m
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/compiler/rtti_out.m,v
> retrieving revision 1.20
> diff -u -t -r1.20 rtti_out.m
> --- compiler/rtti_out.m 2001/01/18 01:18:54 1.20
> +++ compiler/rtti_out.m 2001/02/06 05:47:19
> @@ -296,6 +296,15 @@
> output_generic_rtti_data_defn_start(RttiTypeId,
> du_ptag_ordered_table, DeclSet1, DeclSet),
> io__write_string(" = {\n"),
> + globals__io_lookup_bool_option(reserve_tag, ReserveTag),
> + (
> + { ReserveTag = yes }
> + ->
> + % Output a dummy ptag definition for the var tag first
> + output_dummy_ptag_layout_defns
> + ;
> + []
> + ),
Perhaps s/var tag/reserved tag/ here?
This is the only place that it mentions the purpose of the reserved
tag. I think it would _perhaps_ be better to not mention it here,
since this code doesn't really care what the purpose is, just that
it has been reserved (?).
> output_ptag_layout_defns(PtagLayouts, RttiTypeId),
> io__write_string("\n};\n").
> output_rtti_data_defn(type_ctor_info(RttiTypeId, Unify, Compare,
> @@ -542,6 +551,13 @@
> io__write_string(" },\n")
> ),
> output_ptag_layout_defns(DuPtagLayouts, RttiTypeId).
> +
> + % Output a `dummy' ptag layout, for use by tags that aren't *real*
> + % tags, such as the tag reserved when --reserve-tag is on.
> +:- pred output_dummy_ptag_layout_defns(io__state::di, io__state::uo) is det.
> +
> +output_dummy_ptag_layout_defns -->
> + io__write_string("\t{ 0, MR_SECTAG_NONE, NULL },\n").
Hmm... there's lots of code in the Mercury runtime system which won't
work with such a dummy ptag layout.
What were you planning on doing about that?
I think that warrants at least an XXX comment.
> Index: compiler/type_ctor_info.m
> +++ compiler/type_ctor_info.m 2001/02/15 03:59:14
> @@ -301,9 +302,15 @@
> globals__lookup_bool_option(Globals,
> unboxed_no_tag_types, NoTagOption),
> (
> - NoTagOption = yes,
> + NoTagOption = yes,
> type_constructors_are_no_tag_type(Ctors,
> - Name, ArgType, MaybeArgName)
> + Name, ArgType, MaybeArgName),
> + (
> + ReserveTag = no
> + ;
> + constructors_are_dummy_argument_type(
> + Ctors)
> + )
See my comments for make_tags.m.
The code there seems to duplicate the code from make_tags.m,
perhaps it should be abstracted out into a separate subroutine.
> -type_ctor_info__make_enum_tables(Ctors, ConsTagMap, RttiTypeId,
> +type_ctor_info__make_enum_tables(Ctors, ConsTagMap, RttiTypeId, ReserveTag,
> TypeTables, FunctorInfo, LayoutInfo) :-
> - type_ctor_info__make_enum_functor_tables(Ctors, 0, ConsTagMap,
> + (
> + % XXX Surely this isn't right for enumerations?
> +
> + ReserveTag = yes,
> + InitTag = 1
> + ;
> + ReserveTag = no,
> + InitTag = 0
> + ),
> + type_ctor_info__make_enum_functor_tables(Ctors, InitTag, ConsTagMap,
Reserving tags already disables the use of enums, doesn't it?
So I don't understand the purpose of this code.
It would make more sense to have a call to error/1 or unexpected/2
in the ReserveTag = yes case.
> Index: extras/references/Mmakefile
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/extras/references/Mmakefile,v
> retrieving revision 1.4
> diff -u -t -r1.4 Mmakefile
> --- extras/references/Mmakefile 2000/11/21 23:52:32 1.4
> +++ extras/references/Mmakefile 2001/02/07 04:49:32
> @@ -10,7 +10,7 @@
>
> # Install in an "extras" subdirectory of the main installation tree
> INSTALL_PREFIX := $(INSTALL_PREFIX)/extras
> -LIBGRADES = asm_fast.tr asm_fast.gc.tr.debug
> +LIBGRADES = asm_fast.tr asm_fast.tr.rt asm_fast.gc.tr.rt asm_fast.gc.tr.rt.debug
This change should not go in. Instead, the HAL installation script
that invokes extras/references/Mmakefile should invoke it with `mmake
LIBGRADES=...'. (Or, if this manually rather than by a script,
then the HAL documentation on how to install HAL should be modified.)
> Index: library/builtin.m
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/library/builtin.m,v
> retrieving revision 1.51
> diff -u -t -r1.51 builtin.m
> --- library/builtin.m 2001/02/04 04:10:34 1.51
> +++ library/builtin.m 2001/02/07 05:18:27
> @@ -125,6 +125,7 @@
>
> :- pred unsafe_promise_unique(T, T).
> :- mode unsafe_promise_unique(in, uo) is det.
> +:- mode unsafe_promise_unique(in(any), uo) is det.
That change means that this predicate not only promises uniqueness,
it also promises groundness, which makes the name a misnomer.
So, unless you can come up with some convincing rationale,
this change should not go into the main cvs branch.
> Index: library/private_builtin.m
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/library/private_builtin.m,v
> retrieving revision 1.68
> diff -u -t -r1.68 private_builtin.m
> --- library/private_builtin.m 2001/02/04 04:10:34 1.68
> +++ library/private_builtin.m 2001/02/07 04:56:34
> @@ -1009,6 +1009,7 @@
> :- impure pred var(T).
> :- mode var(ui) is failure.
> :- mode var(in) is failure.
> +:- mode var(in(any)) is failure.
> :- mode var(unused) is det.
It's wrong for the `in(any)' mode of var/1 to always fail.
So, unless you can come up with some convincing rationale,
this change should not go into the main cvs branch.
> Index: library/sparse_bitset.m
> +#ifdef MR_RESERVE_TAG
> +#define ML_BITSET_TAG 1
> +#else
> +#define ML_BITSET_TAG 0
> +#endif
Indent that: s/#define/ #define/g
Also see my early comment in the review of the log message.
> +% XXX this needs to be fixed too
> :- pragma foreign_code("MC++", make_bitset_elem(A::in, B::in) = (Pair::out),
> [will_not_call_mercury, thread_safe],
> "{
The comment there should explain what about it needs to be fixed.
For now, probably the best thing to do there is
#ifdef MR_RESERVE_TAG
#error ...
#else
...
#endif
> Index: library/std_util.m
> +++ runtime/mercury_tags.h 2001/02/08 05:57:37
> @@ -78,9 +78,26 @@
> ** depend on the data representation scheme used by compiler/make_tags.m.
> */
>
> +#ifdef MR_RESERVE_TAG
> +
> +#define MR_RAW_TAG_VAR 0 /* for Prolog-style variables */
> +#define MR_RAW_TAG_NIL 1
> +#define MR_RAW_TAG_CONS 2
> +
> +#define MR_UNIV_TAG 1
> +
> +#else
> +
> #define MR_RAW_TAG_NIL 0
> #define MR_RAW_TAG_CONS 1
>
> +#define MR_UNIV_TAG 0
> +
> +#endif
> +
> +#ifdef MR_RESERVE_TAG
> +#define MR_TAG_VAR MR_mktag(MR_RAW_TAG_VAR)
> +#endif
Please indent all the #defines here by two spaces,
since they're inside #ifdef.
s/MR_UNIV_TAG/MR_RAW_UNIV_TAG/g, to be consistent
with the naming convention for MR_(RAW_)TAG_NIL.
I suggest defining MR_FIRST_UNRESERVED_RAW_TAG as 0 or 1
#ifdef MR_RESERVE_TAG
/* reserve a tag for Prolog-style variables */
#define MR_RAW_TAG_VAR 0
#define MR_FIRST_UNRESERVED_RAW_TAG 1
#else
#define MR_FIRST_UNRESERVED_RAW_TAG 1
#endif
and then defining the others in terms of this:
#define MR_RAW_TAG_NIL MR_FIRST_UNRESERVED_RAW_TAG
#define MR_RAW_TAG_CONS MR_FIRST_UNRESERVED_RAW_TAG + 1
#define MR_UNIV_TAG MR_FIRST_UNRESERVED_RAW_TAG
#define MR_BITSET_ELEM_TAG MR_FIRST_UNRESERVED_RAW_TAG
> Index: runtime/mercury_type_info.h
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/runtime/mercury_type_info.h,v
> retrieving revision 1.64
> diff -u -t -r1.64 mercury_type_info.h
> --- runtime/mercury_type_info.h 2001/02/05 05:19:02 1.64
> +++ runtime/mercury_type_info.h 2001/02/09 02:47:19
> @@ -343,9 +343,15 @@
> ** Definitions for handwritten code, mostly for mercury_compare_typeinfo.
> */
>
> +#ifdef MR_RESERVE_TAG
> +#define MR_COMPARE_EQUAL 1
> +#define MR_COMPARE_LESS 5
> +#define MR_COMPARE_GREATER 9
> +#else
> #define MR_COMPARE_EQUAL 0
> #define MR_COMPARE_LESS 1
> #define MR_COMPARE_GREATER 2
> +#endif
<blink>
What is that change for??
(Also please indent the #defines. But better explain why that change
is needed first...)
> +#define MR_initialise_univ(univ, typeinfo, value) \
> + do { \
> + MR_tag_incr_hp_msg((univ), MR_mktag(MR_UNIV_TAG), \
> + 2, MR_PROC_LABEL, ""std_util:univ/0""); \
> + MR_define_univ_fields((univ), (typeinfo), (value)); \
> } while (0)
I suggest renaming that as MR_new_hp_univ or MR_new_univ_on_hp.
The reason is that this macro accesses the `MR_hp' register, and so
code which calls it needs to make sure that that register is valid.
Putting `hp' in the name will help. It's also worth mentioning that
it a comment here.
> Index: tests/debugger/existential_type_classes.m
>
> my_exist_t = 43.
>
> +:- pragma c_header_code(
> +"
> +").
That looks like a mistake.
> :- pragma c_code(my_univ_value(Univ::in) = (Value::out), will_not_call_mercury, "
> TypeClassInfo_for_existential_type_classes__fooable_T =
> - MR_field(MR_mktag(0), Univ, 0);
> - Value = MR_field(MR_mktag(0), Univ, 1);
> + MR_field(MR_mktag(MR_UNIV_TAG), Univ, 0);
> + Value = MR_field(MR_mktag(MR_UNIV_TAG), Univ, 1);
> ").
>
> :- pragma c_code(my_univ(Value::in) = (Univ::out), will_not_call_mercury, "
> - MR_incr_hp(Univ, 2);
> - MR_field(MR_mktag(0), Univ, 0) =
> + MR_tag_incr_hp(Univ, MR_mktag(MR_UNIV_TAG), 2);
> + MR_field(MR_mktag(MR_UNIV_TAG), Univ, 0) =
> (MR_Word) TypeClassInfo_for_existential_type_classes__fooable_T;
> - MR_field(MR_mktag(0), Univ, 1) = (MR_Word) Value;
> + MR_field(MR_mktag(MR_UNIV_TAG), Univ, 1) = (MR_Word) Value;
Presuming you follow my suggestion above, you'll need
s/MR_UNIV_TAG/MR_UNIV_RAW_TAG/g here and in lots of other places.
Or alternatively, you could #define MR_UNIV_TAG mktag(MR_UNIV_TAG_RAW)
and then s/MR_mktag(MR_UNIV_TAG)/MR_UNIV_TAG/ --
but I suggest doing it the other way first, otherwise the compiler
won't catch any places where you miss replacing MR_UNIV_TAG
with MR_UNIV_RAW_TAG.
> Index: trace/mercury_trace_declarative.c
> +++ trace/mercury_trace_declarative.c 2001/02/12 06:11:40
> @@ -1106,12 +1106,8 @@
> }
>
> MR_TRACE_USE_HP(
> - MR_tag_incr_hp(arg, MR_mktag(0), 2);
> + MR_initialise_univ(arg, arg_type, arg_value);
> );
This is a good example of why it should have `hp' in the name.
Apart from the issues raised above, this looks fine.
--
Fergus Henderson <fjh at cs.mu.oz.au> | "I have always known that the pursuit
| of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh> | -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to: mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions: mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------
More information about the developers
mailing list