[m-dev.] for review: --use-local-vars

[Fergus Henderson]

On 08-Mar-2001, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> -exprn_aux__substitute_lval_in_mem_ref(OldLval, NewLval, MemRef0, MemRef) :-
> +exprn_aux__substitute_lval_in_mem_ref(OldLval, NewLval, MemRef0, MemRef,
> +		N0, N) :-
...
>  	(
> -		MemRef0 = stackvar_ref(N),
> -		MemRef = stackvar_ref(N)
> +		MemRef0 = stackvar_ref(_),
> +		MemRef = MemRef0,

Here you've changed the code to use `_' to avoid the name clash with the
variable `N'.  But I think it might be better to use a more meaningful
name, e.g. `_SlotNum', rather than `_' here.  When reading the code and
trying to verify that it doesn't miss any lvals contained in any of
these terms, it helps if all of the fields are named.

I don't feel very strongly about this.
But please give it careful consideration.

>  	;
> -		MemRef0 = framevar_ref(N),
> -		MemRef = framevar_ref(N)
> +		MemRef0 = framevar_ref(_),
> +		MemRef = MemRef0,

Likewise here.

>	;
> -		MemRef0 = heap_ref(Rval0, Tag, N),
> +		MemRef0 = heap_ref(Rval0, Tag, Slot),
>  		exprn_aux__substitute_lval_in_rval(OldLval, NewLval,
> -			Rval0, Rval),
> -		MemRef = heap_ref(Rval, Tag, N)
> +			Rval0, Rval, N0, N),
> +		MemRef = heap_ref(Rval, Tag, Slot)
>  	).

Here I think "FieldNum" or something along those lines would be a better name
than "Slot", which is normally used to talk about stack slots.

> -exprn_aux__substitute_lval_in_lval_2(OldLval, NewLval, Lval0, Lval) :-
> +exprn_aux__substitute_lval_in_lval_2(OldLval, NewLval, Lval0, Lval, N0, N) :-
>  	(
> -		Lval0 = reg(T, N),
> -		Lval = reg(T, N)
> +		Lval0 = reg(_, _),
> +		Lval = Lval0,

Likewise here, it might be better to keep the names `T' and `N'
(or `_T' and `_N'), or replace them with better ones, e.g. RegType and RegNum.

> -		Lval0 = temp(T, N),
> -		Lval = temp(T, N)
> -	;
> -		Lval0 = stackvar(N),
> -		Lval = stackvar(N)
> -	;
> -		Lval0 = framevar(N),
> -		Lval = framevar(N)
> +		Lval0 = temp(_, _),
> +		Lval = Lval0,
> +		N = N0
> +	;
> +		Lval0 = stackvar(_),
> +		Lval = Lval0,
> +		N = N0
> +	;
> +		Lval0 = framevar(_),
> +		Lval = Lval0,

Likewise here.

> -		Lval0 = lvar(N),
> -		Lval = lvar(N)
> -	).
> +		Lval0 = lvar(_),
> +		Lval = Lval0,

and here.

> +:- pred optimize__maybe_opt_debug(list(instruction)::in, counter::in,
> +	string::in, opt_debug_info::in, opt_debug_info::out,
> +	io__state::di, io__state::uo) is det.
> +
> +optimize__maybe_opt_debug(Instrs, Counter, Msg,
> +		OptDebugInfo0, OptDebugInfo) -->
...
> +		{ string__append_list(["diff -u ",
> +			OptFileName0, " ", OptFileName,
> +			" > ", DiffFileName], DiffCommand) },
> +		io__call_system(DiffCommand, _),

The `-u' option to diff is non-standard.
Not all versions of `diff' support it.

I suggest you use `-c' instead.

> Index: compiler/options.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/options.m,v
> retrieving revision 1.315
> diff -u -b -r1.315 options.m
> --- compiler/options.m	2001/03/05 03:35:26	1.315
> +++ compiler/options.m	2001/03/08 06:45:50
> @@ -380,6 +380,7 @@
>  		;	optimize_jumps
>  		;	optimize_fulljumps
>  		;	checked_nondet_tailcalls
> +		;	use_local_vars
>  		;	optimize_labels
>  		;	optimize_dups
>  %%% unused:	;	optimize_copyprop
> @@ -772,6 +773,7 @@
>  	optimize_jumps		-	bool(no),
>  	optimize_fulljumps	-	bool(no),
>  	checked_nondet_tailcalls	-	bool(no),
> +	use_local_vars		-	bool(yes),

Shouldn't it be off by default?
That's what the comment in your email (preceding the log message) said.

> @@ -2555,6 +2558,8 @@
>  		"\tConvert nondet calls into tail calls whenever possible, even",
>  		"\twhen this requires a runtime check. This option tries to",
>  		"\tminimize stack consumption, possibly at the expense of speed.",
> +		"--no-use-local-vars",
> +		"\tDisable the use of local variables in C code blocks.",

`mmc -O6 --no-use-local-vars' isn't going to do what it is documented to do;
the resulting code will use local variables, because they'll be introduced by
value numbering.

Also if it isn't enabled by default, then the documentation
should document the positive form, not the negative form.

The option should also be documented in doc/user_guide.texi.

> Index: compiler/trace.m
> @@ -800,6 +805,15 @@
>  	set__to_sorted_list(TvarSet, TvarList),
>  	continuation_info__find_typeinfos_for_tvars(TvarList,
>  		VarLocs, ProcInfo, TvarDataMap),
> +
> +	VarLvals = list__map(find_lval_in_var_info, VarInfoList),
> +	map__values(TvarDataMap, TvarLocnSets),
> +	TvarLocnSet = set__union_list(TvarLocnSets),
> +	set__to_sorted_list(TvarLocnSet, TvarLocns),
> +	TvarLvals = list__map(find_lval_in_layout_locn, TvarLocns),
> +	list__append(VarLvals, TvarLvals, LiveLvals),
> +	LiveLvalSet = set__list_to_set(LiveLvals),

A brief comment at the top of this code, e.g.
	
	% compute the live_lvals_info

would be nice.

> +++ use_local_vars.m	Wed Mar  7 13:45:54 2001

That new module should be mentioned in compiler/notes/compiler_design.html.
Likewise for wrap_block.m.

[I haven't reviewed use_local_vars.m yet.
But the rest of this diff looks fine.]

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