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

[Zoltan Somogyi]

On Tue, 2 Oct 2018 01:09:12 +0000 (UTC), Julien Fischer <jfischer at opturion.com> wrote:
> > 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).

I meant the addition of the new option; I did not mean to switch on
all the packing options. (Though that should be coming real soon now.)

> > +:- 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.)

There are very few preprocessor tests for the size of words in the library.
There is one in uint32.m, MR_INT_IS_32_BIT. It is not *exactly* what I want,
since it not being set does not *guarantee* that int is 64 bit (it could be 16, or
128, or some non-power-of-2), but in practice, it ought to work. Do you agree?

> > +    // 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)

Done.

Zoltan.