[m-dev.] for review: update state transformation

[Peter Ross]

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