[m-dev.] for review: improvements/bug fixes for simplify.m

[Fergus Henderson]

On 26-Oct-1999, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> 
> compiler/simplify.m:
> 	Remove unnecessary explicit quantification goals before working
> 	out whether a goal can cause a stack flush.

Fine.

> 	Don't increase the non-locals set of an explicit quantification
> 	goal through common structure elimination because that could change
> 	the determinism.

Fine.

> 	Don't optimize a singleton switch into a test followed
> 	by the case goal if the constructor being tested against
> 	is existentially quantified. This is necessary because
> 	the code to produce the test uses type_util__get_cons_id_arg_types,
> 	which does not correctly handle the existentially quantified
> 	type variables or the type-infos for those type variables.

Ouch.

That change is fine, but I don't think it goes far enough.
I think type_util__get_cons_id_arg_types should be changed to
call error/1 if the cons_id is existentially typed, rather than
quietly returning bogus results.  The documentation for that
predicate should mention that limitation.  And we need to do something
about all the other calls to it. 

Also I think it would be a good idea to have a test case for this
part of the change.

I just had a look, and it turns out that there is only one other
call to type_util__get_cons_id_arg_types.  That call is in
goal_util__case_to_disjunct, which is called only from
goal_util__switch_to_disjunction, which in turn is called only from
magic.m, which is used only by the Aditi interface.  So, does the Aditi
interface support existential types?  What will happen if you try to
write code using existential types and compile it to Aditi-RL?

> Index: compiler/simplify.m
...
> +		    ( 
> +			type_util__get_existq_cons_defn(ModuleInfo1,
> +					Type, ConsId, _)
> +		    ->

That will be a bit inefficient, because type_util__get_existq_cons_defn/4
does quite a bit of work to construct the unused fourth argument;
you might consider writing a predicate type_util__is_existq_cons/3.

>  simplify__goal_2(some(Vars1, CanRemove0, Goal1), SomeInfo,
> +		GoalExpr, GoalInfo, Info0, Info) :-
> +	simplify_info_get_common_info(Info0, Common),
> +	simplify__goal(Goal1, Goal2, Info0, Info1),
> +	simplify__nested_somes(CanRemove0, Vars1, Goal2, SomeInfo, Goal),
> +	Goal = GoalExpr - GoalInfo,
> +	( Goal = some(_, _, _) - _ ->
> +		% Don't increase the set of non-locals of a goal within
> +		% a commit through common structure elimination because
> +		% that may change the determinism.
> +		simplify_info_set_common_info(Info1, Common, Info)
>  	;
> +		Info = Info1
>  	).

>From the comment, I understand roughly what the code is trying to do,
but it took me a long time to understand how the code (set_common_info)
relates to the comment (don't increase the set of non-locals).
I think it would help if you could explain in a little bit more detail
how leaving the common_info unchanged at that point could lead
later code to increase the set of non-locals of this goal.

Also, isn't that test a little conservative?
Not every `some' is a commit.
If the goal already has output variables, then increasing
the set of non-locals won't change the determinism, will it?

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>  |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3        |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------