[m-dev.] for review: removing --args simple

[Fergus Henderson]

On 31-May-1999, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> Remove support for --args simple. We don't use it, we won't use it even for
> experiments, and it is unnecessary complication.

Another reason is that `--args simple' probably doesn't work, due to
code rot and lack of testing ;-)

> compiler/hlds_pred.m:
> 	Don't include an args_method in proc_infos; instead, include
> 	a bool that says whether the procedure's address is taken or not.
> 	(In most cases, this determined whether the args_method was
> 	simple or compact.) We will need this bool in the near future
> 	(when we generate layout structures for procedures whose address
> 	is taken).

I'm not convinced that removing the args_method in proc_infos is a good idea.
It's true that with this change we are only using one argument passing
convention, but that may well change in the future, with high-level C,
bytecode, etc.  Many compilers for other languages, e.g. GNU C, MSVC,
support multiple calling conventions, with ways for the user to declare
that a particular function has a given calling convention.  This is
useful in cases where the function is imported, exported, or has its
address taken.  I think that the likelihood that we will need an args_method
or calling_convention field in proc_infos is quite high.

> compiler/check_typeclass.m:
> compiler/clause_to_proc.m:
> compiler/dnf.m:
> compiler/magic.m:
> compiler/magic_util.m:
> compiler/make_hlds.m:
> compiler/modecheck_call.m:
> compiler/pd_info.m:
> compiler/post_typecheck.m:
> compiler/unify_gen.m:
> 	Minor changes to conform to the changes in the signatures of
> 	the predicates exported from hlds_pred.m.

The merit of these changes depends on the merit of the change
to remove the arg_info field from the pred_info, of which I am
not convinced.

This kind of thing is probably best discussed in person, so I'll
try to catch up with you tomorrow and discuss it.

> Index: compiler/hlds_pred.m
> @@ -1370,15 +1368,13 @@
>  					% analysis.
>  			maybe(list(mode)),
>  					% declared modes of arguments.
> -			args_method
> -					% The args_method to be used for
> -					% the procedure. Usually this will
> -					% be set to the value of the --args
> -					% option stored in the globals. 
> -					% lambda.m will set this field to
> -					% `compact' for procedures it creates
> -					% which must be directly callable by
> -					% a higher_order_call goal.
> +			bool
> +					% Is the address of this procedure
> +					% taken? We must treat such procedures
> +					% differently in at least two aspects.
> +					% First, we must use the compact arg
> +					% passing convention, and second, we
> +					% must use typeinfo liveness for them.

In the comment above, the first aspect no longer makes any sense.
I think the comment should be reworded.

Also I suggest that you explain the rationale for the typeinfo liveness
(e.g. "so that deep_copy() and the garbage collector have the RTTI that
they need for copying closures").

> ===================================================================
> RCS file: /home/mercury1/repository/mercury/library/benchmarking.m,v
> retrieving revision 1.23
> diff -u -b -u -r1.23 benchmarking.m
> --- benchmarking.m	1999/05/26 04:13:53	1.23
> +++ benchmarking.m	1999/05/31 02:05:25
...
> @@ -621,10 +606,10 @@
>  	framevar(0) = r3;
>  	framevar(1) = r4;
>  
> -	if (rep_count <= 0) {
> +	if (r5 <= 0) {
>  		framevar(2) = 1;
>  	} else {
> -		framevar(2) = rep_count;
> +		framevar(2) = r5;
>  	}

I agree with Simon that the readability has gone down.
How about using the register number, but writing a symbolic name
or explanation as a comment, e.g.

		framevar(2) = r5; /* rep_count */

or

		framevar(2) = r5; /* the repetition count */

Incidentally, should that `r5 <= 0' be `(Integer) r5 <= 0'?
The registers are unsigned, so r5 will only ever be == 0,
never < 0.

This comment applies throughout benchmarking.m, but not to
other files -- for most of the other files, the readability
was fine, IMHO.  There were a couple of exceptions which
I have mentioned below.

> Index: runtime/mercury_ho_call.c
...
> @@ -305,21 +265,20 @@
>  
>  	Word	type_ctor_info;
>  
> -	x = mercury__unify__x;
> -	y = mercury__unify__y;
> +	x = r2;
> +	y = r3;
>  
> -	type_ctor_info = field(0, mercury__unify__typeinfo, 0);
> +	type_ctor_info = field(0, r1, 0);
>  	if (type_ctor_info == 0) {
>  		type_arity = 0;
> -		unify_pred = (Code *) field(0, mercury__unify__typeinfo,
> -				OFFSET_FOR_UNIFY_PRED);
> +		unify_pred = (Code *) field(0, r1, OFFSET_FOR_UNIFY_PRED);
>  		/* args_base will not be needed */
>  		args_base = 0; /* just to supress a gcc warning */
>  	} else {
>  		type_arity = field(0, type_ctor_info, OFFSET_FOR_COUNT);
>  		unify_pred = (Code *) field(0, type_ctor_info,
>  				OFFSET_FOR_UNIFY_PRED);
> -		args_base = mercury__unify__typeinfo;
> +		args_base = r1;
>  	}

The use of r1 here is an exception.
I suggest adding

	Word type_info;

	type_info = r1;

at the start and the using `type_info' instead of r1.

>  	save_registers();
> @@ -327,11 +286,10 @@
>  	/* we call `UnifyPred(...ArgTypeInfos..., X, Y)' */
>  	/* virtual_reg(1) will hold the success/failure indication */
>  	for (i = 1; i <= type_arity; i++) {
> -		virtual_reg(i + mercury__unify__offset) =
> -			field(0, args_base, i);
> +		virtual_reg(i) = field(0, args_base, i);
>  	}
> -	virtual_reg(type_arity + mercury__unify__offset + 1) = x;
> -	virtual_reg(type_arity + mercury__unify__offset + 2) = y;
> +	virtual_reg(type_arity + 1) = x;
> +	virtual_reg(type_arity + 2) = y;

I suggest you delete the comment "virtual_reg(1) will hold the success/failure
indication" -- this comment was explaining the "+ 1", which later because
"+ mercury__unify__offset", which you have now deleted.

>  Define_entry(mercury__compare_3_0);
> @@ -464,21 +386,20 @@
>  
>  	Word	type_ctor_info;
>  
> -	x = mercury__compare__x;
> -	y = mercury__compare__y;
> +	x = r2;
> +	y = r3;
>  
> -	type_ctor_info = field(0, mercury__compare__typeinfo, 0);
> +	type_ctor_info = field(0, r1, 0);
>  	if (type_ctor_info == 0) {
>  		type_arity = 0;
> -		compare_pred = (Code *) field(0, mercury__compare__typeinfo,
> -				OFFSET_FOR_COMPARE_PRED);
> +		compare_pred = (Code *) field(0, r1, OFFSET_FOR_COMPARE_PRED);
>  		/* args_base will not be needed */
>  		args_base = 0; /* just to supress a gcc warning */
>  	} else {
>  		type_arity = field(0, type_ctor_info, OFFSET_FOR_COUNT);
>  		compare_pred = (Code *) field(0, type_ctor_info,
>  				OFFSET_FOR_COMPARE_PRED);
> -		args_base = mercury__compare__typeinfo;
> +		args_base = r1;
>  	}

For readability, I suggest adding

	Word type_info;

	type_info = r1;

at the start and the using `type_info' instead of r1.

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