[m-rev.] for review: pack args next to remote sectags

[Julien Fischer]

Hi Zoltan,

On Sun, 26 Aug 2018, Zoltan Somogyi wrote:

> For review by Peter or Julien. The diff is with -b due to systematic
> changes in indentation levels in some files.
>
> The part of this diff that required the most work was the changes
> to the RTTI system. I tried to add test cases for each part of the system
> that was affected by this, from library/construct.m, library/deconstructs.m,
> deep copying and tabling, but there is one affected module for which
> I don't feel comfortable writing tests: library/store.m. If someone
> could add a test case checking the proper operation of store.m
> on terms that use the new data representation, I would appreciate it.

I'll have a go at that.   Based on the XXXs within the store module,
it's probably broken and will be likely more broken after this change.
(Does anyone actually use the generic_ref stuff?)

> Pack subword-sized arguments next to a remote sectag.

...


> diff --git a/compiler/lco.m b/compiler/lco.m
> index 00ffab4..67a7a73 100644
> --- a/compiler/lco.m
> +++ b/compiler/lco.m
> @@ -815,13 +815,12 @@ acceptable_construct_unification(DelayForVars, Goal, !UnifyInputVars, !Info) :-
>      all_true(acceptable_construct_mode(ModuleInfo), ArgModes),
>      get_cons_repn_defn(ModuleInfo, ConsId, CtorRepn),
>      ConsTag = CtorRepn ^ cr_tag,
> -    % The code generator can't handle some kinds of tags. For example, it does
> -    % not make sense to take the address of the field of a function symbol of a
> -    % `notag' type. These are the kinds it CAN handle.
> -    ( ConsTag = single_functor_tag
> -    ; ConsTag = unshared_tag(_)
> -    ; ConsTag = shared_remote_tag(_, _)
> -    ),
> +    % Our optimization is inapplicable to constants, and its implementation
> +    % in the code generator can hanle only some kinds of functors with args.

s/hanle/handle/

> +    % For example, it does not make sense to take the address of the field
> +    % of a function symbol of a `notag' type. The only functors it can handle
> +    % are the ones whose representation is remote_args_tag().
> +    ConsTag = remote_args_tag(_),
>      % If the construction unification has any existential constraints,
>      % then ConstructArgVars will have more elements than the number of
>      % arguments in ConsRepn ^ cr_args (the extra variables will be the

...


> diff --git a/compiler/ml_unify_gen_deconstruct.m b/compiler/ml_unify_gen_deconstruct.m
> index b6755db..94a39e5 100644
> --- a/compiler/ml_unify_gen_deconstruct.m
> +++ b/compiler/ml_unify_gen_deconstruct.m
> @@ -1,4 +1,4 @@
> -%---------------------------------------------------------------------------%
> +%canonicalize, ---------------------------------------------------------------------------%

That doesn't look like it should be there.

>  % vim: ft=mercury ts=4 sw=4 et
>  %---------------------------------------------------------------------------%
>  % Copyright (C) 1999-2012, 2014 The University of Melbourne.
> @@ -236,20 +236,91 @@ ml_generate_det_deconstruction(LHSVar, ConsId, RHSVars, ArgModes, Context,
>              ArgMode, Context, Stmts),
>          Defns = []

...

> diff --git a/compiler/options.m b/compiler/options.m
> index 2b71e2a..73f8f39 100644
> --- a/compiler/options.m
> +++ b/compiler/options.m
> @@ -1454,10 +1454,10 @@ 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_local_sectags         -   bool(no),
> -    allow_packing_remote_sectags        -   bool(no),
> +    allow_packing_dummies               -   bool(yes),
> +    allow_packing_ints                  -   bool(yes),
> +    allow_packing_local_sectags         -   bool(yes),
> +    allow_packing_remote_sectags        -   bool(yes),
>      sync_term_size                      -   int(8),
>                                          % 8 is the size on linux (at the time
>                                          % of writing) - will usually be

Did you intend to enable these by default now?  (If so, that should definitely
be mentioned in the log message.)

The rest of the diff looks ok -- unless Peter has any additional comments
I suggest you go ahead and commit.

Julien