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

[Peter Ross]

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