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

[Peter Wang]

On Thu, 15 Feb 2018 21:23:24 +1100 (AEDT), "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> 
> 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?

Yes.

> > > +%---------------------------------------------------------------------------%
> > > +
> > > +:- 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?

The predicate name doesn't describe what it does. You could rename it,
too. It's not important.

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

Right, thanks.

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

The code marked by the XXX above, which you've now explained.

Peter