[m-rev.] 8, 16 and 32 bit types
Julien Fischer
jfischer at opturion.com
Tue Jul 18 00:08:24 AEST 2017
Hi Zoltan,
On Sun, 16 Jul 2017, Zoltan Somogyi wrote:
> On Sat, 15 Jul 2017 01:15:23 +1000 (AEST), Julien Fischer <jfischer at opturion.com> wrote:
>> There are an updated log message and diff attached.
>
> Apart from the minor quibbles in the attached review,
> the diff is fine. Thank you.
> > diff --git a/compiler/builtin_ops.m b/compiler/builtin_ops.m
> > index 6b1f40fcf..605f70e8c 100644
> > --- a/compiler/builtin_ops.m
> > +++ b/compiler/builtin_ops.m
>
> > (
> > Args = [X, Y, Z],
> > (
> > ProcNum = 0,
> > - Code = assign(Z, binary(int_add, leaf(X), leaf(Y)))
> > + Code = assign(Z, binary(int_add(IntType), leaf(X), leaf(Y)))
>
> Are these lines longer than 79 chars?
They're 80 chars; fixed.
> > - ( PredName = "plus", ArithOp = int_add
> > - ; PredName = "minus", ArithOp = int_sub
> > - ; PredName = "*", ArithOp = int_mul
> > - ; PredName = "times", ArithOp = int_mul
> > - ; PredName = "unchecked_quotient", ArithOp = int_div
> > - ; PredName = "unchecked_rem", ArithOp = int_mod
> > + % XXX should we provide plus/2, minus/2 and times/2 for types other
> > + % than 'int'?
>
> If we don't, then changing the type of something from int to e.g. int32
> could be harder, since calls to e.g. plus would have to be rewritten.
Ok, I've enabled them for all integer types. (I will add the missing
declarations for the uint module along with the second part of this change.)
> > --- a/compiler/foreign.m
> > +++ b/compiler/foreign.m
> > @@ -312,11 +333,32 @@ exported_builtin_type_to_c_string(BuiltinType) = CTypeName :-
> >
...
> > + )
> > ;
> > BuiltinType = builtin_type_float,
> > JavaTypeName = "double"
>
> I don't know either C# or Java well enough to check these translations.
Barring a typo on my type, neither language offers any choice in the matter.
>
> > diff --git a/compiler/inst_check.m b/compiler/inst_check.m
> > index 0f98d574c..710c54b62 100644
> > --- a/compiler/inst_check.m
> > +++ b/compiler/inst_check.m
>
> > @@ -485,8 +532,14 @@ check_for_type_bound_insts(ForTypeKind, [BoundInst | BoundInsts],
> > !:RevMismatchConsIdStrs = [ConsIdStr | !.RevMismatchConsIdStrs]
> > )
> > ;
> > - ( ForTypeKind = ftk_int
> > - ; ForTypeKind = ftk_uint
> > + ( ForTypeKind = ftk_int(int_type_int)
> > + ; ForTypeKind = ftk_int(int_type_uint)
> > + ; ForTypeKind = ftk_int(int_type_int8)
> > + ; ForTypeKind = ftk_int(int_type_uint8)
> > + ; ForTypeKind = ftk_int(int_type_int16)
> > + ; ForTypeKind = ftk_int(int_type_uint16)
> > + ; ForTypeKind = ftk_int(int_type_int32)
> > + ; ForTypeKind = ftk_int(int_type_uint32)
> > ; ForTypeKind = ftk_float
> > ; ForTypeKind = ftk_string
>
> Couldn't those lines be replaced with simply "ForTypeKind = ftk_int(_)"?
Done.
> > + ConsId = uint16_const(_),
> > + (
> > + ForTypeKind = ftk_int(int_type_uint16)
> > + ;
> > + ( ForTypeKind = ftk_user(_, _)
> > + ; ForTypeKind = ftk_int(int_type_int)
> > + ; ForTypeKind = ftk_int(int_type_uint)
> > + ; ForTypeKind = ftk_int(int_type_int8)
> > + ; ForTypeKind = ftk_int(int_type_uint8)
> > + ; ForTypeKind = ftk_int(int_type_int16)
> > + ; ForTypeKind = ftk_int(int_type_int32)
> > + ; ForTypeKind = ftk_int(int_type_uint32)
> > + ; ForTypeKind = ftk_float
> > + ; ForTypeKind = ftk_char
> > + ; ForTypeKind = ftk_string
> > + ),
> > + !:RevMismatchConsIdStrs = [ConsIdStr | !.RevMismatchConsIdStrs]
> > + )
>
> I think these almost-the-same blocks of code are among the rare cases
> where I would say that it is probably better to have an if-then-else
> than a switch, even if --inform-ite-instead-of-switch would complain.
> If we add a new int type, the new value(s) of ConsId will alert us
> (you, when adding int64/uint64) to the need to update this code.
Done.
> > diff --git a/compiler/llds_out_data.m b/compiler/llds_out_data.m
> > index bbcab36f4..a0111f038 100644
> > --- a/compiler/llds_out_data.m
> > +++ b/compiler/llds_out_data.m
> > @@ -548,13 +549,13 @@ output_double_stackvar_ptr(Info, StackType, SlotNum, !IO) :-
> > :- pred llds_types_match(llds_type::in, llds_type::in) is semidet.
> >
> > llds_types_match(Type, Type).
> > -llds_types_match(lt_word, lt_unsigned).
> > -llds_types_match(lt_word, lt_integer).
> > +llds_types_match(lt_word, lt_int(int_type_int)).
> > +llds_types_match(lt_word, lt_int(int_type_int)).
>
> I think one of those should uint, not int.
Indeed, well caught
>
> > @@ -1296,6 +1328,18 @@ output_type_ctor_addr(Module0, Name, Arity, !IO) :-
> > Macro = "MR_INT_CTOR_ADDR"
> > else if Name = "uint" then
> > Macro = "MR_UINT_CTOR_ADDR"
> > + else if Name = "int8" then
> > + Macro = "MR_INT8_CTOR_ADDR"
> > + else if Name = "uint8" then
> > + Macro = "MR_UINT8_CTOR_ADDR"
> > + else if Name = "int16" then
> > + Macro = "MR_INT16_CTOR_ADDR"
> > + else if Name = "uint16" then
> > + Macro = "MR_UINT16_CTOR_ADDR"
> > + else if Name = "int32" then
> > + Macro = "MR_INT32_CTOR_ADDR"
> > + else if Name = "uint32" then
> > + Macro = "MR_UINT32_CTOR_ADDR"
> > else if Name = "float" then
> > Macro = "MR_FLOAT_CTOR_ADDR"
> > else if Name = "string" then
>
> Could this if-then-else chain be converted to a switch and maybe *one*
> if-then-else?
Done.
> > diff --git a/compiler/ml_type_gen.m b/compiler/ml_type_gen.m
> > index 11f5883db..ed41a6ac1 100644
> > --- a/compiler/ml_type_gen.m
> > +++ b/compiler/ml_type_gen.m
> > @@ -452,6 +461,24 @@ ml_gen_constant(Tag, VarType, MLDS_VarType, Rval, !Info) :-
> > Tag = uint_tag(UInt),
> > Rval = ml_const(mlconst_uint(UInt))
> > ;
> > + Tag = int8_tag(Int8),
> > + Rval = ml_const(mlconst_int8(Int8))
> > + ;
> > + Tag = uint8_tag(UInt8),
> > + Rval = ml_const(mlconst_uint8(UInt8))
> > + ;
> > + Tag = int16_tag(Int16),
> > + Rval = ml_const(mlconst_int16(Int16))
> > + ;
> > + Tag = uint16_tag(UInt16),
> > + Rval = ml_const(mlconst_uint16(UInt16))
> > + ;
> > + Tag = int32_tag(Int32),
> > + Rval = ml_const(mlconst_int32(Int32))
> > + ;
> > + Tag = uint32_tag(UInt32),
> > + Rval = ml_const(mlconst_uint32(UInt32))
> > + ;
>
> This is the second occurrence of this code; it is possible it could be
> factored out.
Not without changing the switch that it is part of into an if-then-else. While
this, and the other instances you pointed out, handle integers in the same way,
they handle other tag types differently.
We could modify the cons_tag/0 type so that it has just a single int_tag/1
alternative and then define:
:- type int_tag
---> int_tag(int)
; uint_tag(int)
; ...
I'll look into that as a separate change.
> > diff --git a/compiler/mlds_to_cs.m b/compiler/mlds_to_cs.m
> > index b63593bde..494bcc309 100644
> > --- a/compiler/mlds_to_cs.m
> > +++ b/compiler/mlds_to_cs.m
> > @@ -1905,11 +1913,35 @@ mercury_type_to_string_for_csharp(Info, Type, CtorCat, String, ArrayDims) :-
> > String = "int",
> > ArrayDims = []
> > ;
> > - CtorCat = ctor_cat_builtin(cat_builtin_int),
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_int)),
> > + String = "int",
> > + ArrayDims = []
> > + ;
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_uint)),
> > + String = "uint",
> > + ArrayDims = []
> > + ;
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_int8)),
> > + String = "sbyte",
> > + ArrayDims = []
> > + ;
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_uint8)),
> > + String = "byte",
> > + ArrayDims = []
> > + ;
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_int16)),
> > + String = "short",
> > + ArrayDims = []
> > + ;
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_uint16)),
> > + String = "ushort",
> > + ArrayDims = []
> > + ;
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_int32)),
> > String = "int",
> > ArrayDims = []
> > ;
> > - CtorCat = ctor_cat_builtin(cat_builtin_uint),
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_uint32)),
> > String = "uint",
> > ArrayDims = []
>
> Can this code, and ...
>
> > @@ -3181,11 +3213,29 @@ csharp_builtin_type(Type, TargetType) :-
> > MerType = builtin_type(BuiltinType),
> > require_complete_switch [BuiltinType] (
> > ( BuiltinType = builtin_type_char
> > - ; BuiltinType = builtin_type_int
> > + ; BuiltinType = builtin_type_int(int_type_int)
> > ),
> > TargetType = "int"
> > ;
> > - BuiltinType = builtin_type_uint,
> > + BuiltinType = builtin_type_int(int_type_uint),
> > + TargetType = "uint"
> > + ;
> > + BuiltinType = builtin_type_int(int_type_int8),
> > + TargetType = "sbyte"
> > + ;
> > + BuiltinType = builtin_type_int(int_type_uint8),
> > + TargetType = "byte"
> > + ;
> > + BuiltinType = builtin_type_int(int_type_int16),
> > + TargetType = "short"
> > + ;
> > + BuiltinType = builtin_type_int(int_type_uint16),
> > + TargetType = "ushort"
> > + ;
> > + BuiltinType = builtin_type_int(int_type_int32),
> > + TargetType = "int"
> > + ;
> > + BuiltinType = builtin_type_int(int_type_uint32),
> > TargetType = "uint"
> > ;
>
> ... this code be factored out?
Done.
> > diff --git a/compiler/mlds_to_java.m b/compiler/mlds_to_java.m
> > index 42e16d4c2..ee1b1c4d7 100644
> > --- a/compiler/mlds_to_java.m
> > +++ b/compiler/mlds_to_java.m
>
> > @@ -4585,7 +4618,7 @@ output_std_unop_for_java(Info, UnaryOp, Expr, !IO) :-
> > ; UnaryOp = strip_tag, UnaryOpStr = "/* strip_tag */ "
> > ; UnaryOp = mkbody, UnaryOpStr = "/* mkbody */ "
> > ; UnaryOp = unmkbody, UnaryOpStr = "/* unmkbody */ "
> > - ; UnaryOp = bitwise_complement, UnaryOpStr = "~"
> > + ; UnaryOp = bitwise_complement(int_type_int), UnaryOpStr = "~"
> > ; UnaryOp = logical_not, UnaryOpStr = "!"
> > ; UnaryOp = hash_string, UnaryOpStr = "mercury.String.hash_1_f_0"
> > ; UnaryOp = hash_string2, UnaryOpStr = "mercury.String.hash2_1_f_0"
> > @@ -4593,12 +4626,32 @@ output_std_unop_for_java(Info, UnaryOp, Expr, !IO) :-
> > ; UnaryOp = hash_string4, UnaryOpStr = "mercury.String.hash4_1_f_0"
> > ; UnaryOp = hash_string5, UnaryOpStr = "mercury.String.hash5_1_f_0"
> > ; UnaryOp = hash_string6, UnaryOpStr = "mercury.String.hash6_1_f_0"
> > - ; UnaryOp = uint_bitwise_complement, UnaryOpStr = "~"
> > + ; UnaryOp = bitwise_complement(int_type_uint), UnaryOpStr = "~"
> > + ; UnaryOp = bitwise_complement(int_type_int32), UnaryOpStr = "~"
> > + ; UnaryOp = bitwise_complement(int_type_uint32), UnaryOpStr = "~"
> > ),
> > io.write_string(UnaryOpStr, !IO),
> > io.write_string("(", !IO),
> > output_rval_for_java(Info, Expr, !IO),
> > io.write_string(")", !IO)
>
> Why aren't all the bitwise_complement ops in the same switch arm
> next to each other?
Fixed.
>
> > @@ -4702,18 +4793,56 @@ output_binop_for_java(Info, Op, X, Y, !IO) :-
> > io.write_string(")", !IO)
> > )
> > ;
> > - ( Op = uint_eq
> > - ; Op = uint_ne
> > - ; Op = uint_add
> > - ; Op = uint_sub
> > - ; Op = uint_mul
> > - ; Op = uint_bitwise_and
> > - ; Op = uint_bitwise_or
> > - ; Op = uint_bitwise_xor
> > - ; Op = uint_unchecked_left_shift
> > - ; Op = uint_unchecked_right_shift
> > + ( Op = int_lt(int_type_uint)
> > + ; Op = int_gt(int_type_uint)
> > + ; Op = int_le(int_type_uint)
> > + ; Op = int_ge(int_type_uint)
> > + ; Op = int_lt(int_type_uint32)
> > + ; Op = int_gt(int_type_uint32)
> > + ; Op = int_le(int_type_uint32)
> > + ; Op = int_ge(int_type_uint32)
> > ),
> > - io.write_string("(", !IO),
> > + io.write_string("(((", !IO),
> > + output_rval_for_java(Info, X, !IO),
> > + io.write_string(") & 0xffffffffL) ", !IO),
> > + output_binary_op_for_java(Op, !IO),
> > + io.write_string(" ((", !IO),
> > + output_rval_for_java(Info, Y, !IO),
> > + io.write_string(") & 0xffffffffL))", !IO)
>
> This will output the right thing when the operands are of type uint32.
> Will it do the right thing for operands of type uint on 64 bit systems?
ints and uints in the Java and C# grades are always 32-bits in size regardless
of whether the system is a 32-bit or 64-bit system.
...
> I have the same concerns about 32 bit masks on the other operators below
> as well.
Same thing with those.
> > output_binary_op_for_java(Op, !IO) :-
> > (
> > - ( Op = int_add, OpStr = "+"
> > - ; Op = int_sub, OpStr = "-"
> > - ; Op = int_mul, OpStr = "*"
> > - ; Op = int_div, OpStr = "/"
> > - ; Op = int_mod, OpStr = "%"
> > - ; Op = unchecked_left_shift, OpStr = "<<"
> > - ; Op = unchecked_right_shift, OpStr = ">>"
> > - ; Op = bitwise_and, OpStr = "&"
> > - ; Op = bitwise_or, OpStr = "|"
> > - ; Op = bitwise_xor, OpStr = "^"
> > + ( Op = int_add(_), OpStr = "+"
> > + ; Op = int_sub(_), OpStr = "-"
> > + ; Op = int_mul(_), OpStr = "*"
> > + % NOTE: unsigned div and mode require special handling in Java.
> > + % See output_binop/6 above.
>
> Div and *mod*, not *mode*.
Fixed.
>
> > @@ -4829,6 +4988,24 @@ output_binary_op_for_java(Op, !IO) :-
> > ),
> > io.write_string(OpStr, !IO)
> > ;
> > + Op = unchecked_right_shift(IntType),
> > + (
> > + ( IntType = int_type_int
> > + ; IntType = int_type_int8
> > + ; IntType = int_type_int16
> > + ; IntType = int_type_int32
> > + ),
> > + OpStr = ">>"
> > + ;
> > + (IntType = int_type_uint
> > + ; IntType = int_type_uint8
>
> Indentation.
Fixed.
>
> > diff --git a/compiler/table_gen.m b/compiler/table_gen.m
> > index dc048dbf3..9300a75c4 100644
> > --- a/compiler/table_gen.m
> > +++ b/compiler/table_gen.m
> > @@ -2673,6 +2665,17 @@ gen_lookup_call_for_type(ArgTablingMethod0, CtorCat, Type, ArgVar, VarSeqNum,
> > CodeStr = LookupCodeStr ++ LookupStatsCodeStr ++ UpdateCurNodeCodeStr
> > ).
> >
> > +:- func int_type_to_cat_string(int_type) = string.
> > +
> > +int_type_to_cat_string(int_type_int) = "int".
> > +int_type_to_cat_string(int_type_uint) = "uint".
> > +int_type_to_cat_string(int_type_int8) = "int8".
> > +int_type_to_cat_string(int_type_uint8) = "uint8".
> > +int_type_to_cat_string(int_type_int16) = "int16".
> > +int_type_to_cat_string(int_type_uint16) = "uint16".
> > +int_type_to_cat_string(int_type_int32) = "int32".
> > +int_type_to_cat_string(int_type_uint32) = "uint32".
>
> I think this function may be better off in the module that defines int_type.
Done.
>
> > @@ -3653,12 +3656,30 @@ type_save_category(CtorCat, Name) :-
> > CtorCat = ctor_cat_enum(cat_enum_foreign),
> > sorry($module, $pred, "tabling and foreign enumerations NYI.")
> > ;
> > - CtorCat = ctor_cat_builtin(cat_builtin_int),
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_int)),
> > Name = "int"
> > ;
> > - CtorCat = ctor_cat_builtin(cat_builtin_uint),
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_uint)),
> > Name = "uint"
> > ;
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_int8)),
> > + Name = "int8"
> > + ;
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_uint8)),
> > + Name = "uint8"
> > + ;
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_int16)),
> > + Name = "int16"
> > + ;
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_uint16)),
> > + Name = "uint16"
> > + ;
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_int32)),
> > + Name = "int32"
> > + ;
> > + CtorCat = ctor_cat_builtin(cat_builtin_int(int_type_uint32)),
> > + Name = "uint32"
> > + ;
>
> This code should have a single case for cat_builtin_int that calls
> the function immediately above.
Done.
> > diff --git a/compiler/type_constraints.m b/compiler/type_constraints.m
> > index fcc90cf32..f237ad33c 100644
> > --- a/compiler/type_constraints.m
> > +++ b/compiler/type_constraints.m
> > Name = "int"
> > ;
> > - Type = builtin_type(builtin_type_uint),
> > + Type = builtin_type(builtin_type_int(int_type_uint)),
> > Name = "uint"
> > ;
> > + Type = builtin_type(builtin_type_int(int_type_int8)),
> > + Name = "int8"
> > + ;
> > + Type = builtin_type(builtin_type_int(int_type_uint8)),
> > + Name = "uint8"
> > + ;
> > + Type = builtin_type(builtin_type_int(int_type_int16)),
> > + Name = "int16"
> > + ;
> > + Type = builtin_type(builtin_type_int(int_type_uint16)),
> > + Name = "uint16"
> > + ;
> > + Type = builtin_type(builtin_type_int(int_type_int32)),
> > + Name = "int32"
> > + ;
> > + Type = builtin_type(builtin_type_int(int_type_uint32)),
> > + Name = "uint32"
> > + ;
>
> And it would be useful here as well.
Done.
> > diff --git a/compiler/type_util.m b/compiler/type_util.m
> > index 589525c0a..4537cb541 100644
> > --- a/compiler/type_util.m
> > +++ b/compiler/type_util.m
> > @@ -837,11 +837,29 @@ classify_type_ctor(ModuleInfo, TypeCtor) = TypeCategory :-
> > TypeName = "character",
> > TypeCategoryPrime = ctor_cat_builtin(cat_builtin_char)
> > ;
> > + TypeName = "int",
> > + TypeCategoryPrime = ctor_cat_builtin(cat_builtin_int(int_type_int))
> > + ;
> > TypeName = "uint",
> > - TypeCategoryPrime = ctor_cat_builtin(cat_builtin_uint)
> > + TypeCategoryPrime = ctor_cat_builtin(cat_builtin_int(int_type_uint))
> > ;
> > - TypeName = "int",
> > - TypeCategoryPrime = ctor_cat_builtin(cat_builtin_int)
> > + TypeName = "int8",
> > + TypeCategoryPrime = ctor_cat_builtin(cat_builtin_int(int_type_int8))
> > + ;
> > + TypeName = "uint8",
> > + TypeCategoryPrime = ctor_cat_builtin(cat_builtin_int(int_type_uint8))
> > + ;
> > + TypeName = "int16",
> > + TypeCategoryPrime = ctor_cat_builtin(cat_builtin_int(int_type_int16))
> > + ;
> > + TypeName = "uint16",
> > + TypeCategoryPrime = ctor_cat_builtin(cat_builtin_int(int_type_uint16))
> > + ;
> > + TypeName = "int32",
> > + TypeCategoryPrime = ctor_cat_builtin(cat_builtin_int(int_type_int32))
> > + ;
> > + TypeName = "uint32",
> > + TypeCategoryPrime = ctor_cat_builtin(cat_builtin_int(int_type_uint32))
>
> And if the above function were a predicate, this code could call
> its reverse mode.
Done.
>
> > diff --git a/compiler/xml_documentation.m b/compiler/xml_documentation.m
> > index ae39f9ff8..3d33a02cd 100644
> > --- a/compiler/xml_documentation.m
> > +++ b/compiler/xml_documentation.m
> > @@ -469,8 +469,22 @@ mer_type_to_xml(TVarset, defined_type(TypeName, Args, _)) = Xml :-
> > XmlName = name_to_xml(TypeName),
> > XmlArgs = xml_list("type_args", mer_type_to_xml(TVarset), Args),
> > Xml = elem("type", [Ref], [XmlName, XmlArgs]).
> > -mer_type_to_xml(_, builtin_type(builtin_type_int)) = elem("int", [], []).
> > -mer_type_to_xml(_, builtin_type(builtin_type_uint)) = elem("uint", [], []).
> > +mer_type_to_xml(_, builtin_type(builtin_type_int(int_type_int))) =
> > + elem("int", [], []).
> > +mer_type_to_xml(_, builtin_type(builtin_type_int(int_type_uint))) =
> > + elem("uint", [], []).
> > +mer_type_to_xml(_, builtin_type(builtin_type_int(int_type_int8))) =
> > + elem("int8", [], []).
> > +mer_type_to_xml(_, builtin_type(builtin_type_int(int_type_uint8))) =
> > + elem("uint8", [], []).
> > +mer_type_to_xml(_, builtin_type(builtin_type_int(int_type_int16))) =
> > + elem("int16", [], []).
> > +mer_type_to_xml(_, builtin_type(builtin_type_int(int_type_uint16))) =
> > + elem("uint16", [], []).
> > +mer_type_to_xml(_, builtin_type(builtin_type_int(int_type_int32))) =
> > + elem("int32", [], []).
> > +mer_type_to_xml(_, builtin_type(builtin_type_int(int_type_uint32))) =
> > + elem("uint32", [], []).
>
> This code could use the function above as well.
Also, done.
> > diff --git a/library/private_builtin.m b/library/private_builtin.m
> > index d70f60b0e..e5b509626 100644
> > --- a/library/private_builtin.m
> > +++ b/library/private_builtin.m
> > @@ -51,6 +51,24 @@
> > :- pred builtin_unify_uint(uint::in, uint::in) is semidet.
> > :- pred builtin_compare_uint(comparison_result::uo, uint::in, uint::in) is det.
> >
> > +:- pred builtin_unify_int8(T::in, T::in) is semidet.
> > +:- pred builtin_compare_int8(comparison_result::uo, T::in, T::in) is det.
> > +
> > +:- pred builtin_unify_uint8(T::in, T::in) is semidet.
> > +:- pred builtin_compare_uint8(comparison_result::uo, T::in, T::in) is det.
> > +
> > +:- pred builtin_unify_int16(T::in, T::in) is semidet.
> > +:- pred builtin_compare_int16(comparison_result::uo, T::in, T::in) is det.
> > +
> > +:- pred builtin_unify_uint16(T::in, T::in) is semidet.
> > +:- pred builtin_compare_uint16(comparison_result::uo, T::in, T::in) is det.
> > +
> > +:- pred builtin_unify_int32(T::in, T::in) is semidet.
> > +:- pred builtin_compare_int32(comparison_result::uo, T::in, T::in) is det.
> > +
> > +:- pred builtin_unify_uint32(T::in, T::in) is semidet.
> > +:- pred builtin_compare_uint32(comparison_result::uo, T::in, T::in) is det.
> > +
>
> The Ts look like they should be int8, uint8 etc. Since the signatures
> can't be right yet (since the installed compiler won't have the new types),
> what do we gain by adding these declarations *now*?
We gain the ability to compile test programs involving the new types without
the compiler aborting because it cannot find the above predicates in the
pred_table.
> > diff --git a/runtime/mercury_dotnet.cs.in b/runtime/mercury_dotnet.cs.in
> > index 2e3cb1a7a..7721fe0ee 100644
> > --- a/runtime/mercury_dotnet.cs.in
> > +++ b/runtime/mercury_dotnet.cs.in
> > @@ -338,8 +338,14 @@ public enum TypeCtorRep {
> > MR_TYPECTOR_REP_BITMAP = 44,
> > MR_TYPECTOR_REP_FOREIGN_ENUM = 45,
> > MR_TYPECTOR_REP_FOREIGN_ENUM_USEREQ = 46,
> > - MR_TYPECTOR_REP_UNKNOWN = 47,
> > - MR_TYPECTOR_REP_MAX = 48
> > + MR_TYPECTOR_REP_INT8 = 47,
> > + MR_TYPECTOR_REP_UINT8 = 48,
> > + MR_TYPECTOR_REP_INT16 = 49,
> > + MR_TYPECTOR_REP_UINT16 = 50,
> > + MR_TYPECTOR_REP_INT32 = 51,
> > + MR_TYPECTOR_REP_UINT43 = 52,
> > + MR_TYPECTOR_REP_UNKNOWN = 53,
> > + MR_TYPECTOR_REP_MAX = 54
> > }
> >
> > public class PseudoTypeInfo {
>
> Moving the value of MR_TYPECTOR_REP_UNKNOWN means that once we start using
> the new types, we should bump the binary version compatibility number.
> For now, nothing should be broken except gathering type ctor stats,
> and we can do without that developer tool for now.
Ok.
> > diff --git a/runtime/mercury_tabling_macros.h b/runtime/mercury_tabling_macros.h
> > index 50ee970b3..2c773e973 100644
> > --- a/runtime/mercury_tabling_macros.h
> > +++ b/runtime/mercury_tabling_macros.h
> > @@ -71,10 +71,46 @@
> > #define MR_RAW_TABLE_INT_STATS(stats, table, value) \
> > MR_int_hash_lookup_or_add_stats((stats), (table), (value));
> >
> > -#define MR_RAW_TABLE_UINT(table, value) \
> > +#define MR_RAW_TABLE_UINT(table, value) \
> > MR_word_hash_lookup_or_add((table), (value));
> >
> > -#define MR_RAW_TABLE_UINT_STATS(stats, table, value) \
> > +#define MR_RAW_TABLE_UINT_STATS(stats, table, value) \
> > + MR_word_hash_lookup_or_add_stats((stats), (table), (value));
> > +
> > +#define MR_RAW_TABLE_INT8(table, value) \
> > + MR_int_hash_lookup_or_add((table), (value));
> > +
> > +#define MR_RAW_TABLE_INT8_STATS(stats, table, value) \
> > + MR_int_hash_lookup_or_add_stats((stats), (table), (value));
> > +#define MR_RAW_TABLE_UINT8(table, value) \
> > + MR_word_hash_lookup_or_add((table), (value));
> > +
> > +#define MR_RAW_TABLE_UINT8_STATS(stats, table, value) \
> > + MR_word_hash_lookup_or_add_stats((stats), (table), (value));
>
> There should be an XXX on the 8 bit versions. It doesn't make much sense
> to *hash* -128..127 or 0..255 into a hash table that has 127 or 257 slots
> (the first two hash table sizes).
>
> And I think an explicit cast to MR_Word would be appropriate on the values
> of all the raw tabling macros.
Even the ones for the signed values? I've cast them to MR_Integer.
> Apart from the above minor quibbles, the diff is fine. Thank you.
Thanks for the review.
Julien.
More information about the reviews
mailing list