[m-rev.] for review: add builtin 64-bit integer types -- part 2

[Julien Fischer]

On Tue, 30 Jan 2018, Zoltan Somogyi wrote:

> Attached is my review of the changes to the compiler;
> I will look at the changes to the library tomorrow.

I have fixed a couple of errors in the library since I posted
this change, as given by the folowing diff:

diff --git a/library/int64.m b/library/int64.m
index 6c15948..f037ad9 100644
--- a/library/int64.m
+++ b/library/int64.m
@@ -55,12 +55,12 @@
      %
  :- func abs(int64) = int64.

-    % unchecked_abs(X) retursn the absolute value of X, except that if the
+    % unchecked_abs(X) returns the absolute value of X, except that if the
      % result is undefined if X = int64.min_int64.
      %
  :- func unchecked_abs(int64) = int64.

-    % nabs(X) retursn the negative absolute value of X.
+    % nabs(X) returns the negative absolute value of X.
      % Unlike abs/1 this function is defined for X = int64.min_int64.
      %
  :- func nabs(int64) = int64.
diff --git a/library/uint64.m b/library/uint64.m
index 2d3d120..eff1069 100644
--- a/library/uint64.m
+++ b/library/uint64.m
@@ -193,7 +193,7 @@
  "
      if (I < 0) {
          SUCCESS_INDICATOR = MR_FALSE;
-    } else if ((uint64_t)I > (uint64_t)UINT32_MAX) {
+    } else if ((uint64_t)I > (uint64_t)INT64_MAX) {
          SUCCESS_INDICATOR = MR_FALSE;
      } else {
          U = (uint64_t) I;

> I see that the largest part of the diff is the test cases,
> including .exp files. I don't think any human can really
> check these without dying of boredom.

The main point of all those tests is to ensure we are producing
identical results across all of the different backends.

> Each line of the output now gives the result of a Mercury
> operation on 64 bit ints/uints. How hard would it be to
> follow every line with the result of the same operation,
> but executed in C?

Executing them in C for the non-C backends would be quite difficult ;-)

> Then all we would have to do is to check that every such line pair is
> identical, a task that is trivial to automate.

Ok, I'll see about this as a separate change.

> diff --git a/compiler/c_util.m b/compiler/c_util.m
> index d67f400..4eded72 100644
> --- a/compiler/c_util.m
> +++ b/compiler/c_util.m
> @@ -331,6 +331,7 @@
>
>  :- import_module bool.
>  :- import_module int.
> +:- import_module int64.
>  :- import_module integer.
>  :- import_module require.
>  :- import_module string.
> @@ -894,30 +895,38 @@ output_uint32_expr(Stream, N, !IO) :-
>      io.write_uint32(Stream, N, !IO),
>      io.write_string(Stream, ")", !IO).
> 
> -make_int64_literal(N) = Literal :-
> -    NStr = int_to_string(N),
> -    string.format("INT64_C(%s)", [s(NStr)], Literal).
> -
>  output_uint32_expr_cur_stream(N, !IO) :-
>      io.output_stream(Stream, !IO),
>      output_uint32_expr(Stream, N, !IO).
> 
> +make_int64_literal(N) = Literal :-
> +    ( if N = min_int64 then
> +        Literal = "INT64_MIN"
> +    else
> +        NStr = int64_to_string(N),
> +        string.format("INT64_C(%s)", [s(NStr)], Literal)
> +    ).
> +
> 
> Is there any particular reason why there is make_Uint64_literal?

I don't understand your comment here.

...

> diff --git a/compiler/hlds_out_util.m b/compiler/hlds_out_util.m
> index a047df4..f0d1948 100644
> --- a/compiler/hlds_out_util.m
> +++ b/compiler/hlds_out_util.m
> @@ -2,7 +2,7 @@
>  % vim: ft=mercury ts=4 sw=4 et
>  %-----------------------------------------------------------------------------%
>  % Copyright (C) 2009-2012 The University of Melbourne.
> -% Copyright (C) 2014-2017 The Mercury team.
> +% Copyright (C) 2014-2018 The Mercury team.
>  % This file may only be copied under the terms of the GNU General
>  % Public License - see the file COPYING in the Mercury distribution.
>  %-----------------------------------------------------------------------------%
> @@ -719,13 +719,13 @@ functor_cons_id_to_string(ModuleInfo, VarSet, VarNamePrint, ConsId, ArgVars)
>      ;
>          ConsId = int64_const(Int64),
>          Str = functor_to_string(VarSet, VarNamePrint,
> -            term.integer(base_10, integer(Int64), signed,
> +            term.integer(base_10, integer.from_int64(Int64), signed,
>                  size_64_bit),
>              ArgVars)
>      ;
>          ConsId = uint64_const(UInt64),
>          Str = functor_to_string(VarSet, VarNamePrint,
> -            term.integer(base_10, integer(UInt64), unsigned,
> +            term.integer(base_10, integer.from_uint64(UInt64), unsigned,
>                  size_64_bit),
>              ArgVars)
>      ;
> 
> Why do this via functors?

There is no good reason that I can see.  (The int_const case always did that
and the other cases just copied it.)

> Why not just convert to integer and print that, as
> cons_id_and_vars_or_arity_to_string does?

Done.

> diff --git a/compiler/llds_out_data.m b/compiler/llds_out_data.m
> index 1cb82ed..bb47efd 100644
> --- a/compiler/llds_out_data.m
> +++ b/compiler/llds_out_data.m
> @@ -2021,20 +2021,20 @@ float_op_name(float_divide, "divide").
>      % a name for that rval that is suitable for use in a C identifier.
>      % Different rvals must be given different names.
>      %
> -:- pred int64_literal_name(int::in, string::out) is det.
> +:- pred int64_literal_name(int64::in, string::out) is det.
>
>  int64_literal_name(Int64, Int64Name) :-
> -    Int64Name0 = int_to_string(Int64),
> +    Int64Name0 = int64_to_string(Int64),
>      string.replace_all(Int64Name0, "-", "neg", Int64Name).
>
>      % Given an rval which is an unsigned 64-bit integer literal, return
>      % a name for that rval that is suitable for use in a C identifier.
>      % Different rvals must be given different names.
>      %
> -:- pred uint64_literal_name(int::in, string::out) is det.
> +:- pred uint64_literal_name(uint64::in, string::out) is det.
>
>  uint64_literal_name(UInt64, UInt64Name) :-
> -    UInt64Name = int_to_string(UInt64). % XXX INT64.
> +    UInt64Name = uint64_to_string(UInt64).
> 
> To me, the results of those functions don't look to be suitable
> to use as C identifiers (since most will start with digits).

The wording of the comment is taken from that of float_literal_name.  I have
reworded it and the int64 / uint64 versions thus:

   Given an rval which is an {int64,uint64,float) literal, return a name
   for that rval that is suitable for use as a suffix of a C identifier.
   Different rvals must be given different names.

> diff --git a/compiler/parse_tree_out_info.m b/compiler/parse_tree_out_info.m
> index ff0454d..215ea5f 100644
> --- a/compiler/parse_tree_out_info.m
> +++ b/compiler/parse_tree_out_info.m
> @@ -365,6 +383,18 @@ output_uint32(U32, Str0, Str) :-
>      S = string.uint32_to_string(U32) ++ "u32",
>      string.append(Str0, S, Str).
> 
> +:- pred output_int64(int64::in, string::di, string::uo) is det.
> +
> +output_int64(I64, Str0, Str) :-
> +    S = string.int64_to_string(I64) ++ "i64",
> +    string.append(Str0, S, Str).
> +
> +:- pred output_uint64(uint64::in, string::di, string::uo) is det.
> +
> +output_uint64(U64, Str0, Str) :-
> +    S = string.uint64_to_string(U64) ++ "u64",
> +    string.append(Str0, S, Str).
> +
> 
> These look like they are not used in this module. Are they used anywhere else?

Yes, they are used in one of instances for the output/1 type class further
above in the same module.

Julien.