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

[Zoltan Somogyi]

On Tue, 28 Aug 2018 16:49:15 +1000 (AEST), Julien Fischer <jfischer at opturion.com> wrote:
> > 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?)

Actually, I am *hoping* that it won't be *more* broken after this change.
If the current store.m does not handle sub-word-sized args correctly,
then after this diff that bug will apply to more function symbols
(specifically, those which have a single sub-word-sized arg packed
next to a remote sectag), but I am hoping that I didn't introduce
any new kinds of breakage. However, in the absence of a thorough
test of the functionality of store.m, I cannot be sure.

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

Fixed.

> > 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.

No, it doesn't :-( My keyboard is getting old; keys are getting stuck.
 

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

No. I won't commit the change to this file.
 
> The rest of the diff looks ok -- unless Peter has any additional comments
> I suggest you go ahead and commit.

I want to repeat the bootchecks in 32 bit mode both with and without
packing being enabled before I commit.

Thanks for the review.

Zoltan.