[m-rev.] for review: a pass for computing type representations

[Zoltan Somogyi]

On Tue, 30 Jan 2018 16:11:42 +1100, Peter Wang <novalazy at gmail.com> wrote:
> > As per another part of that same discussion on m-dev, this diff
> > makes a start on implementing a new type of item, the type_repn item,
> > which is intended *only* to be used in compiler-generated interface faces,
> 
> files

Done.

> > +            % In one test case (submodules/direct_arg_cycle1.m), we insert
> > +            % the same value of DirectArgCtors into DirectArgMap0 *twice*.
> > +            %
> > +            % I (zs) don't know whether this is something that we should allow,
> > +            % since one of those is from writing a "where direct_arg is"
> > +            % clause in the *source* code of the program, even though
> > +            % that syntax was intended to be used only in automatically
> > +            % generated interface files.
> > +            %
> > +            % For now, I left the old behavior.
> > +            % XXX TYPE_REPN We should think again about this decision.
> 
> Judging by the commented-out documentation in reference_manual.texi and
> my mail archives, that test case came from an earlier development where
> the direct arg optimisation would not be applied across module boundaries
> without programmer help.
> 
> There was also an idea that direct_arg assertions could serve as checked
> documentation. It doesn't really fit in with the rest of the language so
> I think you can disallow "where direct_arg" clauses in source code
> later.

I agree. I have updated the comment to reflect this.

> > -layout_du_ctor_args(ModuleInfo, DuKind, Ctor0, Ctor) :-
> > -    Ctor0 = ctor(ExistTVars, Constraints, Name, Args0, Arity, Context),
> > +layout_du_ctor_args(ModuleInfo, DuKind, ArgPackBits, CtorRepn0, CtorRepn,
> > +        !CtorRepnMap) :-
> > +    % Args1 is Args0 with any float arg marked as needing a double word
> > +    % if the representation of floats is a double word.
> > +    %
> > +    % Args2 is Args1 with consecutive sub-word-sized arguments packed
> > +    % into single words as much as possible.
> > +    % XXX TYPE_REPN For now, we only recognize enums as sub-word-sized.
> > +    % We should extend this to also cover whichever of int8, int16, int32
> > +    % (and their unsigned versions) are smaller than a word.
> > +    ArgRepns0 = CtorRepn0 ^ cr_args,
> 
> Update the variable names.

Done.

> > diff --git a/compiler/hlds_data.m b/compiler/hlds_data.m
> > index e09aa94..c97d1b0 100644
> > --- a/compiler/hlds_data.m
> > +++ b/compiler/hlds_data.m
> > @@ -94,6 +93,13 @@
> >                  cons_context        :: prog_context
> >              ).
> >  
> > +:- type cons_arg
> > +    --->    cons_arg(
> > +                ca_field_name       :: maybe(ctor_field_name),
> > +                ca_type             :: mer_type,
> > +                ca_context          :: prog_context
> > +            ).
> 
> Unused type?

Yep, it was; it is gone now. Good catch!

> > diff --git a/compiler/make_hlds_passes.m b/compiler/make_hlds_passes.m
> > index 8832ee4..1d6d82c 100644
> > --- a/compiler/make_hlds_passes.m
> > +++ b/compiler/make_hlds_passes.m
> > @@ -430,20 +431,22 @@ do_parse_tree_to_hlds(AugCompUnit, Globals, DumpBaseFileName, MQInfo0,
> >  
> >  add_builtin_type_ctor_special_preds_in_builtin_module(TypeCtor, !ModuleInfo) :-
> >      varset.init(TVarSet),
> > -    Body = hlds_abstract_type(abstract_type_general),
> >      term.context_init(Context),
> >      % These predicates are local only in the public builtin module,
> >      % but we *get here* only if we are compiling the public builtin module.
> >      TypeStatus = type_status(status_local),
> >      construct_type(TypeCtor, [], Type),
> > +    % You cannot construct clauses to unify or compare values of an abstract
> > +    % type. The abstract body is code for "generate code for a builtin type".
> > +    Body = hlds_abstract_type(abstract_type_general),
> 
> s/is code for/is a signal to/

I reworded the comment in a different way.

> > diff --git a/compiler/parse_type_defn.m b/compiler/parse_type_defn.m
> > index 61c24d5..cab871d 100644
> > --- a/compiler/parse_type_defn.m
> > +++ b/compiler/parse_type_defn.m
> ...
> > -:- pred parse_where_type_is_abstract_enum(module_name::in, varset::in,
> > +:- pred parse_where_type_is_abstract(module_name::in, varset::in,
> >      term::in, term::in, prog_context::in, int::in,
> >      maybe1(item_or_marker)::out) is det.
> >  
> > -parse_where_type_is_abstract_enum(ModuleName, VarSet, HeadTerm, BodyTerm,
> > +parse_where_type_is_abstract(ModuleName, VarSet, HeadTerm, BodyTerm,
> >          Context, SeqNum, MaybeIOM) :-
> > +    ContextPieces =
> > +        cord.from_list([words("On the left hand side of type definition:")]),
> >      varset.coerce(VarSet, TypeVarSet),
> > -    parse_type_defn_head(tdhpc_type_defn, ModuleName, VarSet, HeadTerm,
> > +    parse_type_defn_head(ContextPieces, ModuleName, VarSet, HeadTerm,
> >          MaybeNameParams),
> >      ( if
> > -        BodyTerm = term.functor(term.atom("type_is_abstract_enum"), Args, _)
> > +        BodyTerm = term.functor(term.atom(AttrName), Args, _),
> > +        ( AttrName = "type_is_abstract_enum"
> > +        ; AttrName = "type_is_representable_in_n_bits"
> > +        )
> >      then
> > -        ( if
> > -            Args = [Arg],
> > -            decimal_term_to_int(Arg, NumBits)
> > -        then
> > -            TypeDefn0 = parse_tree_abstract_type(abstract_enum_type(NumBits)),
> > -            MaybeTypeDefn = ok1(TypeDefn0)
> > +        ( if Args = [Arg] then
> > +            ( if decimal_term_to_int(Arg, NumBits) then
> > +                TypeDefn0 = parse_tree_abstract_type(
> > +                    abstract_type_fits_in_n_bits(NumBits)),
> > +                MaybeTypeDefn = ok1(TypeDefn0)
> > +            else
> > +                Pieces = [words("Error: the argument of"), quote(AttrName),
> > +                    words("is not an positive integer."), nl],
> 
> a positive

Done.

> > diff --git a/compiler/parse_type_repn.m b/compiler/parse_type_repn.m
> > index e69de29..f542702 100644
> > --- a/compiler/parse_type_repn.m
> > +++ b/compiler/parse_type_repn.m
> > @@ -0,0 +1,280 @@
> > +%-----------------------------------------------------------------------------e
> > +% vim: ft=mercury ts=4 sw=4 et
> > +%-----------------------------------------------------------------------------e
> > +% Copyright (C) 2017 The Mercury team.
> > +% This file may only be copied under the terms of the GNU General
> > +% Public License - see the file COPYING in the Mercury distribution.
> > +%---------------------------------------------------------------------------%
> > +%
> > +% File: parse_type_repn.m.
> > +%
> > +% This module parses XXX

I added a description of the module's job.

> > +
> > +:- module parse_tree.parse_type_repn.
> > +
> > +:- interface.
> > +
> > +:- import_module mdbcomp.sym_name.
> > +:- import_module parse_tree.maybe_error.
> > +:- import_module parse_tree.parse_types.
> > +:- import_module parse_tree.prog_data.
> > +
> > +:- import_module list.
> > +:- import_module term.
> > +:- import_module varset.
> > +
> > +    % Parse ":- type_representation(..., ..., ...)" items.
> > +    %
> 
> Too many args.

Fixed.
 
> > +% However, in many cases, we *do* need to know the representation of the type.
> > +% For example, we need that information
> > +%
> > +% - to decide whether an eqv type is equivalent to a dummy type;
> > +% - to decide whether an arguments of a functor of a du type are dummies; and
> 
> s/an arguments/arguments

Done.

> > diff --git a/compiler/write_module_interface_files.m b/compiler/write_module_interface_files.m
> > index 1965b54..e7afaea 100644
> > --- a/compiler/write_module_interface_files.m
> > +++ b/compiler/write_module_interface_files.m
> > @@ -939,8 +950,11 @@ make_impl_type_abstract(TypeDefnMap, !TypeDefnPairs) :-
> >              % Leave dummy types alone.
> >              true
> >          else
> > -            ( if du_type_is_enum(Ctors, NumBits) then
> > -                Details = abstract_enum_type(NumBits)
> > +            ( if du_type_is_enum(DetailsDu, NumBits) then
> > +                % XXX TYPE_REPN We should also generate fitns_in_n_bits
> > +                % if the original type is a less-than-word-sized builtin,
> > +                % such as int8.
> > +                Details = abstract_type_fits_in_n_bits(NumBits)
> >              else
> >                  Details = abstract_type_general
> >              ),
> 
> s/fitns/fits
>

Done.
 
> I can't give more than a superficial review but deciding type
> representations in a single pass is obviously a good idea.

Thank you for the review above.

Julien, would you be willing to review it, preferably tomorrow?
An updated diff is attached. If not, I would prefer to commit it
without a thorough review, so I can start working on the many
"XXX TYPE_REPN" added by this diff.

Zoltan.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: DIFF.du2.gz
Type: application/x-gzip
Size: 123289 bytes
Desc: not available
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20180130/a96ea605/attachment-0001.bin>