[m-rev.] For review: (no attachments) Extensions to constraints based mode analysis.

[Ralph Becket]

Throughout you use `ie' instead of `i.e.,'.  If you want full points
for style, you only use `i.e.,' in parentheses, otherwise write out
`that is' (or `id est' if you're a classical buff).

> Index: compiler/abstract_mode_constraints.m
> ===================================================================
> +%-----------------------------------------------------------------------------%
> +
> +    % equiv_no(Context, MCVar, !Constraints) constrains MCVar to `no' in
> +    % Constraints. Context should be the context of the goal or
> +    % declaration that imposed this constraint.
> +    %
> +:- pred equiv_no(prog_context::in, mc_var::in, pred_p_c_constraints::in,
> +    pred_p_c_constraints::out) is det.

Why just `equiv_no' and not `equiv_yes'?  Why not make the no/yes
binding a parameter?

> +    % xor(Context, A, B, !Constraints) constrains mode constraint variables A
> +    % and B in Constraints by the constraint (A xor B), ie constrains exactly
> +    % one of them to be true. Context should be the context of the goal or
> +    % declaration that imposed this constraint.
> +    %
> +:- pred xor(prog_context::in, mc_var::in, mc_var::in, pred_p_c_constraints::in,
> +    pred_p_c_constraints::out) is det.

I don't think `xor' is a good name, but I can't think of anything better
off hand.

> 
>  %-----------------------------------------------------------------------------%
>  %-----------------------------------------------------------------------------%
> 
>  :- implementation.
> 
> +:- import_module parse_tree.error_util.
> +
> +:- import_module int.
>  :- import_module string.
> 
>  %-----------------------------------------------------------------------------%
> 
> +    % `unit'-like type used to distinguish mode constraint variables.
> +    %
>  :- type mc_type ---> mc_type.

This is usually referred to as a "dummy type".

> +abstract_mode_constraints.add_constraint(Context,
> +    ConstraintFormula, !PredConstraints) :-

There should be an extra level of indentation on the line above.

> +    AllProcsConstraints = !.PredConstraints ^ pred_constraints,
> +    ConstraintAnnotation = constraint_annotation(Context),
> +    FormulaAndAnnotation = pair(ConstraintFormula, ConstraintAnnotation),
> +    !:PredConstraints = !.PredConstraints ^ pred_constraints :=
> +        list.cons(FormulaAndAnnotation, AllProcsConstraints).

Why `list.cons(...)' rather than just
`[FormulaAndAnnotation | AllProcsConstraints]'?

> +    % Prints the variable bindings of this solution, one per line.
> +    %
> +:- pred pretty_print_bindings(mc_varset::in, mc_bindings::in,
> int::in, int::out,
> +    io::di, io::uo) is det.
> +
> +pretty_print_bindings(VarSet, Bindings, N, N + 1, !IO) :-
> +    io.write_string("Solution " ++ string.from_int(N) ++ ":\n{\n", !IO),
> +    Variables = map.keys(Bindings),
> +    list.foldl((pred(Var::in, IO0::di, IO::uo) is det :-
> +            io.write_string("    ", IO0, IO1),
> +            io.write_string(varset.lookup_name(VarSet, Var), IO1, IO2),
> +            io.write_string(" = ", IO2, IO3),
> +            map.lookup(Bindings, Var, Value),
> +            io.print(Value, IO3, IO4),
> +            io.nl(IO4, IO)
> +        ),
> +        Variables, !IO),
> +    io.write_string("}\n", !IO).

That would be clearer if the lambda was replaced by a named predicate.

> Index: compiler/build_mode_constraints.m
> ===================================================================
> @@ -72,99 +80,114 @@
> 
>      % An abstract representation of a mode constraint variable.
>      %
> -    % It represents the constraint variable that determines whether
> -    % the program variable specified is produced at the goal path
> -    % specified.
> +    % ProgVar `in` PredId, `at` GoalPath represents the constraint
> +    % variable of the proposition that ProgVar is produced at
> +    % goal GoalPath in predicate PredId.

Do you mean to include that comma before `at`?

>      % mode_decls_constraints(ModuleInfo, VarMap, PredId, Decls,
> -    % HeadVarsList, Constraints)
> +    %   HeadVarsList, Constraints)
>      %
>      % Constraints is the disjunction of the constraints for
>      % individual declared modes being satisfied.  ie
>      % disj([ConstraintsForMode1, ..., ConstraintsForModen])
>      %
> -    % Note that if Decls is an empty list, this is interpreted as
> -    % there being no modes for which the predicate can be executed
> -    % and the returned constraints are [disj([])] which cannot be
> -    % satisfied. Mode declarations for predicates with no declared
> -    % modes should not be handled by this - they must be handled
> -    % elsewhere.
> -    %
>      % The constraints for a predicate with declared modes is the

Do you mean for a predicate or for a call?

>      % disjunction of the constraints for each declared mode. If,
>      % according to the mode declaration, a variable is not initially



> +add_goal_constraints(ModuleInfo, ProgVarset, PredId, GoalExpr - GoalInfo,
> +        !VarInfo, !Constraints) :-
>      goal_info_get_nonlocals(GoalInfo, Nonlocals),
>      goal_info_get_goal_path(GoalInfo, GoalPath),
> +    goal_info_get_context(GoalInfo, Context),
> +    add_goal_expr_constraints(ModuleInfo, ProgVarset, PredId,
> GoalExpr, Context,

Is this an indentation error?

> +add_goal_expr_constraints(ModuleInfo, ProgVarset, CallerPredId, GoalExpr,
> +        Context, GoalPath, _Nonlocals, !VarInfo, !Constraints) :-
> +    GoalExpr = call(CalleePredId, _, Args, _, _, _),
> +    module_info_pred_info(ModuleInfo, CalleePredId, CalleePredInfo),
> +
> +    ( pred_info_infer_modes(CalleePredInfo) ->
> +        % No modes declared so just constrain the hearvars

s/hearvars/headvars/

> +    % A variable bound at any disjunct is bound for the disjunction
> +    % as a whole. If a variable can be bound at one disjunct
> +    % it must be able to be bound at any.

Grammar correction:

% A variable bound by any disjunct is bound by the disjunction
% as a whole. If a variable is bound by one disjunct then
% it must be bound by all of them.

> Index: compiler/check_hlds.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/check_hlds.m,v
> retrieving revision 1.10
> diff -u -r1.10 check_hlds.m
> --- compiler/check_hlds.m	12 Oct 2005 23:51:34 -0000	1.10
> +++ compiler/check_hlds.m	25 Jan 2006 04:41:54 -0000
> @@ -38,8 +38,6 @@
> 
>  % Mode analysis
>  %:- module mode_analysis.
> -   :- include_module abstract_mode_constraints.
> -   :- include_module build_mode_constraints.
>     :- include_module delay_info.
>     :- include_module inst_match.
>     :- include_module inst_util.
> @@ -53,9 +51,18 @@
>     :- include_module modecheck_call.
>     :- include_module modecheck_unify.
>     :- include_module modes.
> -   :- include_module prop_mode_constraints.
>     :- include_module unify_proc.
>     :- include_module unique_modes.
> +
> +   % XXX This doesn't belong here but we don't know where it's home is at
> +   % the moment.
> +   %
> +   :- include_module abstract_mode_constraints.
> +   :- include_module build_mode_constraints.
> +   :- include_module mcsolver.
> +   :- include_module ordering_mode_constraints.
> +   :- include_module prop_mode_constraints.

I think these do belong here: modes.m is a submodule of check_hlds.

> Index: compiler/ordering_mode_constraints.m (new file)
> ===================================================================
> 
>     % add_ordering_constraint(Constraint, !OCI) adds Constraint
>     % to the ordering constraints store. It fails if it immediately
>     % detects a contradiction (at the moment, this means it has
>     % detected a loop in the producer/consumer dependency graph).
>     %
> :- pred add_ordering_constraint(mode_ordering_constraint::in,
>     ordering_constraints_info::in, ordering_constraints_info::out) is semidet.
> 
>     % add_lt_constraint(A, B, !OCI) constrains conjunct A to come
>     % before conjunct B, in the constraints store.
>     %
> :- pred add_lt_constraint(conjunct_id::in, conjunct_id::in,
>     ordering_constraints_info::in, ordering_constraints_info::out) is semidet.

Why do we need both of these predicates?

> mode_reordering(PredConstraintsMap, VarMap, SCCs, !ModuleInfo) :-
>     (
>         list.foldl(scc_reordering(PredConstraintsMap, VarMap), SCCs,
>             !ModuleInfo)
>     ->
>         true
>     ;
>         % XXX Until mode inference is complete and the final
>         % structure of this module is decided this is all that
>         % can be done.
>         sorry(this_file, "mode ordering failure")
>     ).

Hmm.  Do you have to bail out?  Can't you mark the pred. as "to be
handled by the old mode ordering mechanism"?  This situation occurs
frequently in your diff.

> %-----------------------------------------------------------------------------%
> 
>     % add_ordering_constraint(Constraint, OCI0, OCI) adds Constraint
>     % to the ordering constraints store. It fails if it immediately
>     % detects a contradiction (at the moment, this means it has
>     % detected a loop in the producer consumer dependency graph - eg

s/eg/e.g.,/ - as with `i.e.,'.

>     % prog_var_ordering_constraints(VarMap, Bindings, ProgVar, RepVars,
>     %   !OCInfo)
>     %
>     % Adds ordering constraints for a conjunction to OCInfo.
>     % Specifically, those relating to which conjuncts produce and which
>     % consume the variable ProgVar. See conjunct_ordering_constraints
>     % for details.
>     %
> :- pred prog_var_ordering_constraints(mc_var_map::in, mc_bindings::in,
>     prog_var::in, list(mc_rep_var)::in,
>     ordering_constraints_info::in, ordering_constraints_info::out) is semidet.

What about impurity related ordering constraints?

>     % original_order_constraints(N, MOCs) produces a list of constraints MOCs
>     % that describe a complete order for N conjuncts, such that they are not
>     % reordered at all from their original positions.
>     %
> :- pred original_order_constraints(int::in,
>     mode_ordering_constraints::out) is det.
> 
> original_order_constraints(N, MOCs) :-
>     complete_order_constraints(1 `..` N, MOCs).

Since (..) is already an infix operator, you don't need the backquotes.

>     % add_complete_order_constraints(Xs, !MOCs) adds a list of constraints
>     % that describe a compete order for Xs such that it is not reordered
>     % at all.
>     %
> :- pred add_complete_order_constraints(list(conjunct_id)::in,
>     set(mode_ordering_constraint)::in,
>     set(mode_ordering_constraint)::out) is det.
> 
> add_complete_order_constraints([], !MOCs).
> add_complete_order_constraints([Conjunct | Conjuncts], !MOCs) :-
>     list.foldl(insert_lt_constraint(Conjunct), Conjuncts, !MOCs),
>     add_complete_order_constraints(Conjuncts, !MOCs).

Do you need all O(n^2) ordering constraints?  Can't you do it with O(n)?

>     % constraint_if_possible(Constraints, !OCI)

s/constraint_if_possible/constrain_if_possible/

> Index: compiler/mcsolver.m (new file)
> ===================================================================

>     % For debugging purposes only.
> % :- pred main(io :: di, io :: uo) is det.

Do you still want this here?

Otherwise that's fine and you should check it in!

Cheers,
-- Ralph
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------