for review: deforestation [1/4]

[Fergus Henderson]

On 19-Feb-1998, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> +* We've added a new source-to-source transformation - deforestation.
> +
> +  Deforestation transforms conjunctions to avoid the construction
> +  of intermediate data structures and to avoid multiple traversals
> +  over data structures. Deforestation is enabled with the
> +  `--deforestation' option.

I suggest

     ... Deforestation is enabled at optimization level `-O3' or higher,
     or by using the `--deforestation' option.

> Index: compiler/goal_util.m

You should document the predicates which you have exported from
this module.  (The standard for documentation is higher for
exported predicates than it is for predicates which are local
to a module.)

> Index: compiler/hlds_goal.m
> @@ -682,10 +682,24 @@
> +       % Union together all the nonlocals of a list of goals.
> +:- pred goal_list_nonlocals(list(hlds_goal), set(var)).
> +:- mode goal_list_nonlocals(in, out) is det.

I suggest s/Union together/Return the union of/

> Index: compiler/inlining.m
> +:- pred inlining__do_inline_call(list(var), pred_info, proc_info, 
> +	varset, varset, map(var, type), map(var, type),
> +	tvarset, tvarset, map(tvar, type_info_locn), 
> +	map(tvar, type_info_locn), hlds_goal).
> +:- mode inlining__do_inline_call(in, in, in, in, out, in, out,
> +	in, out, in, out, out) is det.

This predicate should be documented.

> +inlining__do_inline_call(ArgVars, PredInfo, ProcInfo, 
> +		VarSet0, VarSet, VarTypes0, VarTypes, TypeVarSet0, TypeVarSet, 
> +		TypeInfoVarMap0, TypeInfoVarMap, Goal) :-
> +
> +	proc_info_goal(ProcInfo, CalledGoal),
> +
> +	% look up the rest of the info for the called procedure.
> +
> +	pred_info_typevarset(PredInfo, CalleeTypeVarSet),
> +	proc_info_headvars(ProcInfo, HeadVars),
> +	proc_info_vartypes(ProcInfo, CalleeVarTypes0),
> +	proc_info_varset(ProcInfo, CalleeVarset),

Please be consistent with capitalization -- either VarSet or Varset
but not both.  (I prefer the former.)

> Index: compiler/intermod.m
...
> +			;
> +				{ Deforestation = yes },
> +				% Double the inline-threshold since
> +				% goals we want to deforest will have
> +				% at least two disjuncts. This allows 
> +				% one simple goal in each disjunct.
> +				{ DeforestThreshold is InlineThreshold * 2 },
> +				{ inlining__is_simple_goal(Goal,
> +					DeforestThreshold) },
> +				{ goal_is_deforestable(PredId, Goal) }

Shouldn't that be

	{ DeforestThreshold is InlineThreshold * 2 + 1 },

instead of

	{ DeforestThreshold is InlineThreshold * 2 },

?  The disjunction itself will add one to the goal size.

> Index: compiler/mercury_compile.m
> @@ -712,7 +712,17 @@
>  	mercury_compile__maybe_polymorphism(HLDS25, Verbose, Stats, HLDS26),
>  	mercury_compile__maybe_dump_hlds(HLDS26, "26", "polymorphism"), !,
>  
> -	mercury_compile__maybe_termination(HLDS26, Verbose, Stats, HLDS28),
> +	{ HLDS27 = HLDS26 },
> +	%mercury_compile__check_unique_modes(HLDS26, Verbose, Stats,
> +	%		HLDS27, FoundUniqError), !,
> +
> +	%{ FoundUniqError = yes ->
> +	%	error("unique modes failed")
> +	%;
> +	%	true
> +	%},

If you want to leave that commented out code in there,
then you should include a comment explaining what it does
and/or why it has been commented out.

options.m:
> +	io__write_string("\t--deforestation-depth-limit\n"),
> +	io__write_string("\t\tSpecify a depth limit for the deforestation algorithm\n"),
> +	io__write_string("\t\tin addition to the usual termination checks.\n"),
> +	io__write_string("\t\tA value of -1 specifies no depth limit.\n"),
> +	io__write_string("\t--deforestation-vars-threshold\n"),
> +	io__write_string("\t\tSpecify a rough limit on the number of variables\n"),
> +	io__write_string("\t\tin a procedure created by deforestation.\n"),
> +	io__write_string("\t\tA value of -1 specifies no limit.\n").

What are the default limits?

Apart from the comments above, everything else in part 1 looks fine.
Stay tuned for part 2...

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