[m-dev.] for review: recognise associativity assertions

[Peter Ross]

Fergus,

Is it alright to check this code in?

On 09-Dec-1999, Peter Ross <petdr at cs.mu.OZ.AU> wrote:
> On 09-Dec-1999, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> > On 17-Nov-1999, Peter Ross <petdr at cs.mu.OZ.AU> wrote:
> > > Recognise associativity assertions, and use them to introduce
> > > accumulators.
> > ...
> > > mercury/compiler/accumulator.m:
> > >     Rather than failing if a call is associative and nothing is known
> > >     about the effect of rearranging the argument order, report a
> > >     suppressible warning.
> > 
> > Hmm, interesting...
> > 
> > What can the user do if for one warning, they want to not just
> > suppress the warning but also prevent the argument rearrangement,
> > while for another warning, they want to just supress the warning?
> > 
> Place the two predicates in separate files.  It is an all or nothing at
> the moment.
> 
> > > +++ accumulator.m	1999/11/17 03:53:30
> > > @@ -222,8 +230,46 @@
> > >  			[]
> > >  		),
> > >  
> > > -		{ ProcInfo   = ProcInfo1 },
> > > -		{ ModuleInfo = ModuleInfo1 }
> > > +		globals__io_lookup_bool_option(inhibit_accumulator_warnings,
> > > +				InhibitWarnings),
> > > +		(
> > > +			( { Warnings = [] } ; { InhibitWarnings = yes } )
> > > +		->
> > > +			{ ModuleInfo = ModuleInfo1 }
> > > +		;
> > > +			{ error_util__describe_one_pred_name(ModuleInfo1,
> > > +					PredId, PredName) },
> > > +			{ pred_info_context(PredInfo, Context) },
> > > +
> > > +			error_util__write_error_pieces(Context, 0,
> > > +					[words("In the"), words(PredName)]),
> > 
> > Probably you should delete the word "the" their, for consistency
> > with error messages from other parts of the compiler.
> > 
> Ok.
> 
> > > +			{ proc_info_varset(ProcInfo, VarSet) },
> > > +			accumulator__warnings(Warnings, VarSet, ModuleInfo1),
> > > +
> > > +			error_util__write_error_pieces(Context, 2, 
> > > +					[
> > > +					words("Please ensure that these"),
> > > +					words("argument rearrangements"),
> > > +					words("do not introduce"),
> > > +					words("performance problems."),
> > > +					words("These warnings can"),
> > > +					words("be suppressed by"),
> > > +					words("--inhibit-accumulator-warnings.")
> > 
> > Put the option name inside `single quotes'.
> > 
> Done.
> 
> > > +:- pred accumulator__warning(warning::in, prog_varset::in, module_info::in,
> > > +		prog_context::out, list(format_component)::out) is det.
> > > +
> > > +accumulator__warning(w(Context, PredId, VarA, VarB), VarSet, ModuleInfo,
> > > +		Context, Formats) :-
> > > +	error_util__describe_one_pred_name(ModuleInfo, PredId, PredStr),
> > > +	varset__lookup_name(VarSet, VarA, VarAStr),
> > > +	varset__lookup_name(VarSet, VarB, VarBStr),
> > > +	Formats = [words("warning: The"), words(PredStr),
> > > +			words("has had the location of the variables"),
> > > +			words(VarAStr), words("and"), words(VarBStr),
> > > +			words("swapped to allow accumulator introduction.")
> > > +			].
> > 
> > I think you should probably replace "The" with "the call to".
> > 
> > I can anticipate that this warning will confuse a lot of users,
> > so you should definitely include a verbose expanation that is
> > enabled by --verbose-errors.
> > 
> 
> How about:
>     If a predicate has been declared associative via a promise
>     declaration, the compiler is required to rearrange the order of that
>     call to the predicates arguments if it makes the containing
>     predicate tail recursive.  If this changes the performance
>     characteristics of the call to the predicate, use
>     `--no-accumulator-introduction' to turn the optimization off, or
>     `--inhibit-accumulator-warnings' to turn ignore the warnings.
> 
> > It would be a good idea to have a test case in tests/warnings
> > to test this warning.
> > 
> Will do when this code is checked in.
> 
> > > +assertion__is_associativity_assertion(AssertId, Module, CallVars,
> > > +		AssociativeVars) :-
> > > +	assertion__goal(AssertId, Module, Goal - GoalInfo),
> > > +	equivalent(Goal - GoalInfo, P, Q),
> > > +
> > > +	goal_info_get_nonlocals(GoalInfo, UniversiallyQuantifiedVars),
> > > +
> > > +	P = some(_, _, conj(PCalls) - _) - _PGoalInfo,
> > > +	Q = some(_, _, conj(QCalls) - _) - _QGoalInfo,
> > > +
> > > +	AssociativeVars = promise_only_solution(associative(PCalls, QCalls,
> > > +				UniversiallyQuantifiedVars, CallVars)).
> > 
> > I think the code here should have a comment explaining why the
> > call to promise_only_solution is needed and why it is guaranteed
> > to only have one solution (if, indeed, it is -- from a brief perusal
> > of the code it wasn't immediately clear to me that that was the case).
> > 
> There should be only one or zero solutions.  The call to list__perm
> ensures this, as the pairs are not symmetric.
> 
> list__perm(Pairs, [(A - AB) - (B - A), (B - C) - (C - BC),
>                 (AB - ABC) - (BC - ABC)]),
> 
> 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
> --------------------------------------------------------------------------
----
Peter Ross
PhD Student University of Melbourne
http://www.cs.mu.oz.au/~petdr/
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------