[m-rev.] for review: decide each type's representation just once

[Zoltan Somogyi]

On Thu, 15 Feb 2018 16:17:22 +1100, Peter Wang <novalazy at gmail.com> wrote:

> On Thu, 15 Feb 2018 10:36:07 +1100 (AEDT), "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> > Peter, can you please review this, preferably sometime soon?
> > 
> > The parts of the diff concerned with automatically comparing
> > the output of the new algorithm with the output of the old
> > are not intended to be committed. I included them both to show
> > how I did that comparison, and to keep the functionality in case
> > any changes in response to review comments require
> > retesting.
> 
> You could commit it for a few days (however long it takes to produce a
> source distribution) so that we can check for any differences on other
> programs.

You mean including the copy of the original du_type_layout.m as
du_type_layout_old.m, and the auto-comparison code?

> > +:- type maybe_unboxed_no_tag_types
> > +    --->    no_unboxed_no_tag_types
> > +    ;       use_unboxed_no_tag_types.
> 
> Maybe we should standardise on "notag" to avoid ambiguity.

OK, but given that the rest of the compiler has lots of references
to both notag and no_tag types, it will have to be a separate change.

> > +%---------------------------------------------------------------------------%
> > +
> > +:- pred are_direct_args_enabled(globals::in, compilation_target::in,
> > +    maybe_direct_args::out, direct_arg_map::in, direct_arg_map::out) is det.
> > +
> > +are_direct_args_enabled(Globals, Target, MaybeDirectArgs, !DirectArgMap) :-
> > +    (
> > +        Target = target_c,
> > +        globals.lookup_bool_option(Globals, record_term_sizes_as_words,
> > +            TermSizeWords),
> > +        globals.lookup_bool_option(Globals, record_term_sizes_as_cells,
> > +            TermSizeCells),
> > +        ( if
> > +            TermSizeWords = no,
> > +            TermSizeCells = no
> > +        then
> > +            MaybeDirectArgs = direct_args_enabled
> > +        else
> > +            % We cannot use direct arg functors in term size grades.
> > +            MaybeDirectArgs = direct_args_disabled,
> > +            map.init(!:DirectArgMap)
> > +        )
> > +    ;
> > +        ( Target = target_csharp
> > +        ; Target = target_java
> > +        ; Target = target_erlang
> > +        ),
> > +        % Direct arg functors have not (yet) been implemented on these targets.
> > +        MaybeDirectArgs = direct_args_disabled,
> > +        map.init(!:DirectArgMap)
> > +    ).
> >  
> 
> I suggest the caller clears DirectArgMap itself.

May I ask why do you suggest that?

As an XXX TYPE_REPN says, I was considering replacing the
MaybeDirectArgs/DirectArgMap pair with a type isomorphic to a
maybe(direct_arg_map), effectively deleting the map instead of
clearing it. Would that address your concerns?

> > +decide_if_simple_du_type(ModuleInfo, Params, TypeCtorToForeignEnumMap,
> > +        TypeCtorTypeDefn0, TypeCtorTypeDefn, !MustBeSingleFunctorTagTypes,
> > +        !ComponentTypeMap, !NoTagTypeMap, !Specs) :-
> > +    TypeCtorTypeDefn0 = TypeCtor - TypeDefn0,
> > +    get_type_defn_body(TypeDefn0, Body0),
> >      (
> > +        Body0 = hlds_du_type(Ctors, MaybeCanonical, MaybeRepn0,
> >              MaybeForeign),
> >          expect(unify(MaybeRepn0, no), $pred, "MaybeRepn0 != no"),
> > +        expect(negate(unify(Ctors, [])), $pred, "Ctors != []"),
> 
> You could use expect_not.

Done.

> > +:- pred add_du_if_ctor_is_word_aligned_ptr(decide_du_params::in,
> > +    type_ctor::in, hlds_type_defn::in, maybe(foreign_type_body)::in,
> > +    set_tree234(type_ctor)::in, set_tree234(type_ctor)::out,
> > +    component_type_map::in, component_type_map::out) is det.
> 
> add_du_if_single_ctor_is_word_aligned_ptr?

Done.

> > +add_du_if_ctor_is_word_aligned_ptr(Params, TypeCtor, TypeDefn, MaybeForeign,
> > +        !MustBeSingleFunctorTagTypes, !ComponentTypeMap) :-
> > +    % Are we guaranteed to choose a word aligned pointer as the representation?
> > +    ( if
> > +        TypeCtor = type_ctor(_TypeCtorSymName, TypeCtorArity),
> > +
> > +        % NOTE We could let the argument's type to have a set of type params
> > +        % that is a subset of the type params of the containing type,
> > +        % but that would require the runtime system to be able to handle
> > +        % variables in the argument type, during unification and comparison
> > +        % (mercury_unify_compare_body.h) during deconstruction
> > +        % (mercury_ml_expand_body.h), during deep copying
> > +        % (mercury_deep_copy_body.h), and maybe during some other
> > +        % operations.
> > +        TypeCtorArity = 0,
> > +
> > +        % XXX TYPE_REPN Why this test?
> > +        DirectArgMap = Params ^ ddp_direct_arg_map,
> > +        not map.search(DirectArgMap, TypeCtor, _DirectArgFunctors)
> 
> Hmm, I suppose this corresponded to something in the old algorithm?

Yes, it duplicates a test in the legacy algorithm (in a slightly different form,
due to changes in data structures).

The XXX is there because the compiler itself should never mark such a functor
as a direct_arg functor, because if a type has a single functor (we get here only in that case)
and the functor has one argument (direct_arg functors always have arity 1),
then the type is a notag type, and the direct_arg optimization does not apply to it.
And if it was the user who added the "where direct_arg is" clause (before we take
such clauses away from them), we shouldn't silently ignore the clause, as the code
above is doing; we should generate an error message instead.

> The rest looked fine. Just that part in
> add_du_if_ctor_is_word_aligned_ptr confuses me.

By "that part", do you mean the predicate name, the XXX above,
or something else?

Zoltan.