[m-rev.] for review: arg_vectors (part 1)

[Mark Brown]

On 18-Dec-2006, Julien Fischer <juliensf at csse.unimelb.edu.au> wrote:
> 
> For review by Mark or Zoltan.
> 
> Estimated hours taken: 14
> Branches: main
> 
> Add a new data structure, called an arg_vector, to the compiler that is
> intended to encapsulate the argument passing conventions that we use.
> (Primarily those described in the comments at the head of polymorphism.m).
> The intention is to use this new data structure everywhere we currently use
> lists to represent procedure and call site argument information.

Could you list the other places that will eventually use this data structure?
Is it only the proc_info, or are there others (e.g., the parse tree)?

> 
> This change adds the new data structure plus supporting utility predicates.
> It also begins the task of moving the code in the compiler over to the new
> data structure.  Specifically, this diff change the headvars field in the

s/change/changes/

> clauses_info structure into an arg_vector.
> 
> The spots marked XXX ARGVEC will need to be further modified once the
> proc_info structure has been modified to use arg_vectors.
> 
> compiler/hlds_args.m:
> 	New module.  This module defines the arg_vector structure that will
> 	eventually be used to represent procedure and call site argument
> 	information throughout the compiler.  It also defines some utility
> 	procedures that operate on that data structure.
> 
> compiler/hlds.m:
> 	Include the new module in the HLDS package.
> 
> compiler/hlds_clauses.m:
> 	Modify the clauses_info structure to represent clause arguments as
> 	arg_vectors rather than lists.
> 
> 	Adapt the predicates that initialise the clauses_info structure so
> 	that they correctly handle the arg_vector structure's representation
> 	of function return values.
> 
> compiler/polymorphism.m:
> 	Use the arg_vector data structure when introducing type_infos and
> 	typeclass_info arguments.

Add "into the heads of clauses" or something like that.  This change doesn't
cover the case of introducing them in calls in the body.

> Index: compiler/hlds_args.m
> ===================================================================
> RCS file: compiler/hlds_args.m
> diff -N compiler/hlds_args.m
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ compiler/hlds_args.m	18 Dec 2006 05:31:52 -0000
> @@ -0,0 +1,326 @@
> +%-----------------------------------------------------------------------------%
> +% vim: ft=mercury ts=4 sw=4 et
> +%-----------------------------------------------------------------------------%
> +% Copyright (C) 2006 The University of Melbourne.
> +% This file may only be copied under the terms of the GNU General
> +% Public License - see the file COPYING in the Mercury distribution.
> +%-----------------------------------------------------------------------------%
> +%
> +% File: hlds_args.m.
> +% Main authors: juliensf.
> +%
> +% This module defines the part of the HLDS that deals with procedure and 
> call
> +% site arguments.  (See comments at the head of polymorphism.m for further
> +% details.)

I think you should move the relevant comments here, starting from "The
argument passing convention is..."

You may need to fix some references elsewhere in polymorphism.m.

> +%
> +%-----------------------------------------------------------------------------%
> +
> +:- module hlds.hlds_args.
> +:- interface.
> + 
> +:- import_module mdbcomp.prim_data.
> + 
> +:- import_module list.
> +:- import_module map.
> +:- import_module maybe.
> +:- import_module set.
> +
> +%-----------------------------------------------------------------------------%
> +
> +	% This type represents the arguments to a predicate symbol, or the
> +	% arguments and result of a function symbol.  The arguments may be
> +	% variables, types, modes, etc, depending on the context.
> +	%
> +	% Rather than keep all arguments in a single list, we retain
> +	% information about the origin of each argument (such as whether
> +	% it was introduced by polymorphism.m to hold a type_info).  This
> +	% simplifies the processing in polymorphism.m, and also abstracts
> +	% away the specific calling convention that we use.
> +    %
> +:- type arg_vector(T).

Since it applies to procedure arguments, why not call it proc_args?  If we
later add a similar data structure for data constructors, which also have
an argument convention, what will it be called?

(I think I asked you this in person, but I can't remember the answer.)

> +%-----------------------------------------------------------------------------%
> +%
> +% Utility predicates that operate on arg_vectors
> +%
> +
> +    % Return a list of the items in a arg_vector.  The order of the
> +    % list corresponds to that required by the calling conventions.
> +    % See comments at the head of polymorphism.m. for details.  If the
> +    % arg_vector represents a function then the last element in the list

s/represents/is for/

> +    % will correspond to the function return value.

That last sentence is really part of the calling convention, and can be
omitted.  It should be added in the documentation above, though.


> +    % 
> +:- func arg_vector_to_list(arg_vector(T)) = list(T).
> + 
> +    % Return the set of items in an arg_vector.
> +    %
> +:- func arg_vector_to_set(arg_vector(T)) = set(T).
> +
> +    % Apply a renaming to an arg_vector.
> +    % Useful for renaming variables etc.
> +    % 
> +:- pred apply_renaming_to_arg_vector(map(T, T)::in,
> +    arg_vector(T)::in, arg_vector(T)::out) is det.
> +
> +    % partition_arg_vector_by_origin(Vec, PolyArgs, NonPolyArgs):
> +    %
> +    % Partition the argument vector into two lists depending on whether
> +    % something was introduced by the polymorphism transformation or not.
> +    %
> +:- pred partition_arg_vector_by_origin(arg_vector(T)::in, list(T)::out,
> +    list(T)::out) is det.

partition_arg_vector_poly_args?

split_arg_vector_poly_args?

> +%-----------------------------------------------------------------------------%
> +%-----------------------------------------------------------------------------%
> +
> +:- implementation.
> +
> +:- import_module libs.compiler_util.
> +:- import_module parse_tree.prog_util.
> +:- import_module parse_tree.prog_type. 
> +    % Require for apply_partial_map_to_list
> +    % XXX that should really live in a different module.

In map.m, I'd suggest.

> +
> +:- import_module string.
> +
> +%-----------------------------------------------------------------------------%
> +
> +    % The first six fields are set by the polymorphism pass.
> +    %
> +:- type arg_vector(T)
> +    --->    arg_vector(
> +                av_instance_type_infos :: list(T),
> +                % Type_infos for the unconstrained type variables in an 
> +                % instance declaration.

...in the order that they first appear in the instance arguments.

> +arg_vector_init(PredOrFunc, Args0) = ArgVec :-
> +    (
> +        PredOrFunc = predicate,
> +        Args = Args0,
> +        MaybeRetVal = no
> +    ;
> +        PredOrFunc = function,
> +        pred_args_to_func_args(Args0, Args, RetVal),

Why not just use list.det_split_last?  The pred args vs func args abstraction
is not needed here, particularly since the comment for arg_vector_init
states that the function result should be last.

If this data structure is intended to be used throughout the compiler, then
pred_args_to_func_args should eventually become obsolete.

> Index: compiler/polymorphism.m
> ===================================================================
> RCS file: 
> /home/mercury/mercury1/repository/mercury/compiler/polymorphism.m,v
> retrieving revision 1.310
> diff -u -r1.310 polymorphism.m
> --- compiler/polymorphism.m	7 Dec 2006 15:31:12 -0000	1.310
> +++ compiler/polymorphism.m	15 Dec 2006 03:51:58 -0000
> @@ -462,19 +463,10 @@
>      map.lookup(PredTable0, PredId, PredInfo0),
>      pred_info_clauses_info(PredInfo0, ClausesInfo0),
>      clauses_info_get_vartypes(ClausesInfo0, VarTypes0),
> -    clauses_info_get_headvars(ClausesInfo0, HeadVars),
> +    clauses_info_get_headvars(ClausesInfo0, HeadVarVec),

Since the field is still called "headvars", I'd keep that for the variable
names as well.  Use HeadVarList in places where a list is still used.

Other than that, this change is fine.

Cheers,
Mark.

--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------