[m-rev.] for review: bulk unify and compare

[Julien Fischer]

Hi Zoltan,

On Mon, 1 Oct 2018, Zoltan Somogyi wrote:

> This diff, which is for review by anyone, implements Mantis
> feature request 468.


> Unify and compare packed args in bulk when possible.

...

> diff --git a/compiler/options.m b/compiler/options.m
> index 9e7cf45..c5da34e 100644
> --- a/compiler/options.m
> +++ b/compiler/options.m
> @@ -1458,12 +1458,13 @@ option_defaults_2(compilation_model_option, [
>                                          % all word bits for argument packing.
>      allow_double_word_fields            -   bool(yes),
>      allow_double_word_ints              -   bool(no),
> -    allow_packing_dummies               -   bool(no),
> -    allow_packing_ints                  -   bool(no),
> -    allow_packing_chars                 -   bool(no),
> -    allow_packing_local_sectags         -   bool(no),
> -    allow_packing_remote_sectags        -   bool(no),
> +    allow_packing_dummies               -   bool(yes),
> +    allow_packing_ints                  -   bool(yes),
> +    allow_packing_chars                 -   bool(yes),
> +    allow_packing_local_sectags         -   bool(yes),
> +    allow_packing_remote_sectags        -   bool(yes),
>      allow_packing_mini_types            -   bool(no),
> +    allow_packed_unify_compare          -   bool(yes),
>      sync_term_size                      -   int(8),

Since it's not mentioned in the log message, I assume that you didn't
mean the above (yet).

...

> diff --git a/library/private_builtin.m b/library/private_builtin.m
> index a8f2b52..6a4fb03 100644
> --- a/library/private_builtin.m
> +++ b/library/private_builtin.m

...

> +:- pragma foreign_proc("C",
> +    compare_remote_int32_bitfields(TermVarX::in, TermVarY::in,
> +        Ptag::in, CellOffsetVar::in, ShiftVar::in, ResultVar::uo),
> +    [will_not_call_mercury, thread_safe, promise_pure,
> +        does_not_affect_liveness, may_not_duplicate],
> +"
> +#ifdef MR_INT_LEAST64_TYPE

If your intention was to use that macro to distinguish between 32- and 64-bit
systems, then that won't work.  (MR_INT_LEAST64_TYPE *will* be defined
on 32-bit systems so long as they also provide a 64-bit integer type.)

> +    // All uses of this predicate should override the body,
> +    // but just in case they don't ...
> +    MR_Unsigned *cell_x;
> +    MR_Unsigned *cell_y;
> +    cell_x = (MR_Unsigned *) (((MR_Unsigned) TermVarX) - (MR_Unsigned) Ptag);
> +    cell_y = (MR_Unsigned *) (((MR_Unsigned) TermVarY) - (MR_Unsigned) Ptag);
> +    MR_Unsigned word_x = cell_x[CellOffsetVar];
> +    MR_Unsigned word_y = cell_y[CellOffsetVar];
> +    int32_t value_x = (int32_t) ((word_x >> ShiftVar) & ((1L << 32) - 1));
> +    int32_t value_y = (int32_t) ((word_y >> ShiftVar) & ((1L << 32) - 1));

The expression (1L << 32) will cause GCC to emit a warning on platforms
where sizeof(long) == 4 (e.g. Windows).  The portable (in C99) way to get 1 as a
64-bit (signed) integer literal would to use the INT64_C macro, e.g.

       (INT64_C(1) << 32)

> +    if (value_x < value_y) {
> +        ResultVar = MR_COMPARE_LESS;
> +    } else if (value_x > value_y) {
> +        ResultVar = MR_COMPARE_GREATER;
> +    } else {
> +        ResultVar = MR_COMPARE_EQUAL;
> +    }
> +#else
> +    MR_fatal_error(""compare_remote_int32_bitfields called on ""
> +        ""non-64-bit system"");
> +#endif
> +").

Ditto for compare_local_int32_bitfields below.

The rest looks fine.

Julien.