[m-dev.] for review: update state transformation
Peter Ross
petdr at cs.mu.OZ.AU
Tue Apr 11 17:50:33 AEST 2000
On Tue, Apr 11, 2000 at 01:08:44PM +1000, Fergus Henderson wrote:
> On 29-Mar-2000, Peter Ross <petdr at cs.mu.OZ.AU> wrote:
> > Index: compiler/accumulator.m
> ...
> > %
> > % For each member of the assoc set determine the substitutions
> > % needed, and also check the efficiency of the procedure isn't
> > % worsened by swapping reordering the arguments to a call.
> > %
>
> s/swapping reordering/reordering/
>
Done.
> > process_assoc_set([Id | Ids], GS, OutPrime, ModuleInfo, Substs0, Types0,
> > Substs, Types, CS, Warnings) :-
> ...
> > % ONLY swap the order of the variables if the goal is
> > % associative.
> > (
> > IsCommutative = yes,
> > CSGoal = Goal - InstMap,
> > Warning = []
> > ;
> > IsCommutative = no,
>
> Is that comment correct?
> I had difficulty relating the comment to the code;
> the comment talks about associativity, but the code is
> testing commutativity.
>
No it should read that the order of the variables only needs to be
swapped if the goal doesn't have the additional property of
commutativity.
> > :- pred heuristic(module_name::in, string::in, arity::in, prog_vars::in,
> > set(prog_var)::out) is semidet.
> >
> > heuristic(unqualified("list"), "append", 3, [_Typeinfo, A, _B, _C], Set) :-
> > set__list_to_set([A], Set).
>
> You should document the semantics of this predicate.
>
% heuristic returns the set of which head variables are
% important in the running time of the predicate.
I have also added the comment
% Only do the transformation if the
% accumulator variable is *not* in a
% position where it will control the
% running time of the predicate.
where the heuristic call is actually used.
> >
> > % inefficient order so that accumulators can be
> > % introduced.
> > append(StateOutputVars0, [StateOutputVar], StateOutputVars),
> > append(Accs0, [Acc], Accs),
> > append(BasePairs0, [StateOutputVar - Acc0], BasePairs).
>
> That comment was a bit cryptic for me -- could you change it
> to explain things in a little more detail?
>
% concatenate to the end of the list. This allows the
% accumulator introduction to be applied as the heuristic
% will succeed (remember after transforming the two input
% variables will have their order swapped, so they must be
% in the inefficient order to start with)
> compiler/assert.m:
> > assertion__is_associativity_assertion(AssertId, Module, CallVars,
> > - AssociativeVars) :-
> > + AssociativeVars, OutputVar) :-
> ...
> > + % There may or may not be a some [] depending on whether
> > + % the user explicity quatified the call or not.
>
> s/quatified/qualified/
>
Done.
> > +process_one_side(Goals, UniversiallyQuantifiedVars, PredId,
> > + LinkingVar, Vars, VarsA) :-
> > + process_two_linked_calls(Goals, UniversiallyQuantifiedVars, PredId,
> > + LinkingVar, Vars0, VarsA),
> > +
> > + % Filter out all the invariant arguments, and then make
> > + % sure that their is only 3 arguments left.
>
> s/their is/there are/
>
Done.
> > +update(PCalls, QCalls, UniversiallyQuantifiedVars, CallVars, StateA - StateB) :-
> ...
> > + % If you read the predicate documentation, you will note
> > + % that for each pair of variables on the left hand side
> > + % their is an equivalent pair of variables on the right
> > + % hand side.
>
> s/their/there/
>
Done.
> > +++ goal_store.m Tue Jan 11 15:44:56 2000
> > @@ -0,0 +1,114 @@
> > +% Module: goal_store
> > +% Main authors: petdr
> > +%
> > +% Given a hlds_goal store it in a goal_structure.
>
> Did you mean goal_store rather than goal_structure?
>
> In any case, it would be very helpful to explain what the purpose
> storing a goal in a goal_store/goal_struct is.
>
% Define a type goal_store(Key) which allows a hlds_goal to be stored in
% a dictionary like structure. However there some operations on this
% dictionary which are specific to hlds_goals.
> > +++ library/list.m 2000/03/14 00:08:00
> > @@ -81,6 +81,11 @@
> > ( some [BC]
> > (list__append(B, C, BC), list__append(A, BC, ABC)) )
> > ).
> > + % construction equivalence law.
> > + % XXX when we implement rewrite rules change this law to a
> > + % rewrite rule.
> > +:- promise all [L,H,T] ( append([H], T, L) <=> L = [H|T] ).
>
> I suggest changing the wording of that slightly:
> s/rules change/rules, we should change/
>
Done.
> That completes my review; apart from the comments above and in the earlier
> messages that I posted, this change looks fine.
>
I will do a cvs update and make sure it all still bootchecks and then
send off a relative diff and commit.
Thanks for the quick review.
Pete
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to: mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions: mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------
More information about the developers
mailing list