[m-rev.] for review: stack slot optimization, part 4

[Fergus Henderson]

On 09-Mar-2002, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Index: compiler/matching.m
> +% Stability: low

No need for a "Stability:" comment for modules in the compiler directory.

> +:- module matching.
> +
> +:- interface.
> +:- import_module prog_data.

It would be nicer to delete that import, and use `var(T)' instead of
`prog_var' throughout, if that is possible.

> +:- import_module bool, list, set.
> +
> +%-----------------------------------------------------------------------------%
> +
> +% find_via_cell_vars(CellVar, CandidateFieldVars, CellVarFlushedLater,
> +%	BeforeFlush, AfterFlush, CellVarStoreCost, CellVarLoadCost,
> +%	FieldVarStoreCost, FieldVarLoadCost,
> +%	OpRatio, NodeRatio, InclAllCand,
> +%	RealizedBenefitNodes, RealizedCostNodes, ViaCellVars)
> +%
> +% CellVar gives a variable that corresponds to a memory cell, while
> +% CandidateArgVars gives a subset of the variables that are the fields
> +% of that cell. BeforeFlush gives the set of variables the program accesses
> +% in the segment before the first stack flush, while each element of AfterFlush
> +% corresponds to a segment, and gives the set of variables accessed in that
> +% segment. CellVarStoreCost, CellVarStoreCost, FieldVarStoreCost and
> +% FieldVarStoreCost give the relative costs of the four kinds of operations

The 2nd and 4th occurrences of "Store" should be "Load".

It is probably worth grouping these four costs as a struct (with named
fields), so they are passed as a single argument, just to simplify the
interface a little.

> +reachable_by_alternating_path(SelectedCostNodes, Graph, Matching,
> +		BenefitNodes0, BenefitNodes) :-
...
> +		set__union(AdjBenefitNodes, BenefitNodes0, BenefitNodes1),
> +		% XXX should reverse the first two args
> +		set__difference(BenefitNodes0, AdjBenefitNodes,
> +			NewBenefitNodes),

I don't understand the XXX comment.

> Index: compiler/optimize.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/optimize.m,v
> retrieving revision 1.28
> diff -u -b -r1.28 optimize.m
> --- compiler/optimize.m	31 May 2001 05:59:50 -0000	1.28
> +++ compiler/optimize.m	8 Nov 2001 00:19:54 -0000
> @@ -364,8 +364,10 @@
>  			[]
>  		),
>  		globals__io_lookup_int_option(num_real_r_regs, NumRealRRegs),
> +		globals__io_lookup_int_option(local_var_access_threshold,
> +			AccessThreshold),
>  		{ use_local_vars__main(Instrs3, Instrs,
> -			ProcLabel, NumRealRRegs, C2, C) },
> +			ProcLabel, NumRealRRegs, AccessThreshold, C2, C) },
>  		optimize__maybe_opt_debug(Instrs, C, "after use_local_vars",
>  			OptDebugInfo3, OptDebugInfo)
>  	;

That change wasn't mentioned in the log message.

> Index: compiler/options.m
> @@ -2678,6 +2737,8 @@
>  %		"\tThis option has no effect unless the `--high-level-code' option",
>  %		"\tis enabled.  It also has no effect if the `--target' option is",
>  %		"\tset to `il'.",
> +%		"--no-opt-no-return-calls",
> +%		"\tDo not optimize the stack usage of calls that cannot return.",

There should be a comment saying why the documentation for
that option is commented out.

Also, for the other new options which are deliberately not documented in
the help message, there should be a comment in the source explaining what
they are for.

> Index: compiler/pragma_c_gen.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/pragma_c_gen.m,v
> retrieving revision 1.48
> diff -u -b -r1.48 pragma_c_gen.m
> --- compiler/pragma_c_gen.m	13 Feb 2002 09:56:25 -0000	1.48
> +++ compiler/pragma_c_gen.m	14 Feb 2002 06:32:04 -0000
> @@ -31,7 +31,7 @@
>  :- pred pragma_c_gen__generate_pragma_c_code(code_model::in,
>  	pragma_foreign_proc_attributes::in, pred_id::in, proc_id::in,
>  	list(prog_var)::in, list(maybe(pair(string, mode)))::in, list(type)::in,
> -	hlds_goal_info::in, pragma_foreign_code_impl::in, code_tree::out,
> +	pragma_foreign_code_impl::in, hlds_goal_info::in, code_tree::out,
>  	code_info::in, code_info::out) is det.

Uh, what was the motivation for swapping those two arguments?

That change wasn't mentioned in the log message.

> Index: compiler/stack_alloc.m
> ===================================================================
> RCS file: compiler/stack_alloc.m
> diff -N compiler/stack_alloc.m
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ compiler/stack_alloc.m	19 Feb 2002 09:30:47 -0000
> @@ -0,0 +1,194 @@
> +%-----------------------------------------------------------------------------%
> +% Copyright (C) 2002 The University of Melbourne.
> +% This file may only be copied under the terms of the GNU General
> +% Public License - see the file COPYING in the Mercury distribution.
> +%-----------------------------------------------------------------------------%
> +%
> +% File live_vars.m

No, it's stack_alloc.m.

Otherwise this part looks fine.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------