[m-dev.] for review: compressing RTTI using non-word creates (part 2)

[Fergus Henderson]

On 26-Apr-1999, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> +++ stack_layout.m	1999/04/26 05:23:13
...
> +% The field containing the number of live data items is encoded by the
> +% formula (#Long << 8 + #Short), where #Short is the number data items whose
> +% descriptions fit into an MR_Short_Lval and #Long is the number of data
> +% items whose descriptions do not.

What happens if #Short > 255?

I think you should detect that case and handle it appropriately.

> +stack_layout__construct_liveval_arrays(VarInfos, LengthRval,
> +		TypeLocnVector, NameVector) -->
...
> +	{ stack_layout__byte_bits(ByteBits) },
> +	{ EncodedLength is IntArrayLength << ByteBits + ByteArrayLength },

Why `byte_bits'?  Is there any reason why representing things as e.g.
"#Long << 10 + #Short" wouldn't work?  If not, then the named constant
that names this hard-coded shift count should be named something other
than `byte_bits'. 

(Also, it would probably be better to use a zero-arity function rather
than a unary predicate for named constants like this.)

> +:- pred stack_layout__make_tagged_byte(int::in, int::in, int::out) is semidet.
> +
> +stack_layout__make_tagged_byte(Tag, Value, TaggedValue) :-
> +	stack_layout__byte_bits(ByteBits),
> +	int__pow(2, ByteBits - 2, Limit),
> +	Value < Limit,
> +	TaggedValue is unchecked_left_shift(Value, 2) + Tag.

I suggest replacing the last two occurrences of the constant `2' by
a named constant, called say `short_locn_tag_bits' or something like that.

> +int
> +MR_get_register_number_short(MR_Short_Lval locn)
> +{
> +	if ((locn & 03) == MR_SHORT_LVAL_TYPE_R) {
> +		return locn >> 2;

Likewise here I would suggest writing that as

	/* in header file */
	#define MR_SHORT_LOCN_TAG_BITS 2
	
	/* in C file */
	if ((locn & ((1 << MR_LOCN_TAG_BITS) - 1)) == MR_SHORT_LVAL_TYPE_R) {
		return LOCN >> MR_LOCN_TAG_BITS;
	}

It would be even better to use a bit more abstraction, e.g.
something like

	if (MR_SHORT_LOCN_TAG(locn) == MR_SHORT_LVAL_TYPE_R) {
		return MR_SHORT_LVAL_R_REG(locn);

Here the MR_SHORT_LVAL_R_REG(n) is the one that you defined
in mercury_stack_layout.h, and MR_SHORT_LOCN_TAGS is a new macro
which would be defined in mercury_stack_layout.h.

Maybe MR_SHORT_LVAL_TAGS would be a better name than MR_SHORT_LOCN_TAGS,
I'm not sure.

>  bool
> -MR_get_type_and_value_base(const MR_Stack_Layout_Var *var,
> +MR_get_type_and_value_base(const MR_Stack_Layout_Vars *vars, int i,
>  	Word *saved_regs, Word *base_sp, Word *base_curfr,
>  	Word *type_params, Word *type_info, Word *value)
>  {
>  	bool	succeeded;
>  	Word	*pseudo_type_info;
> -	int	i;
>  
> -	if (!MR_LIVE_TYPE_IS_VAR(var->MR_slv_live_type)) {
> +	if (!MR_LIVE_TYPE_IS_VAR(MR_var_pti(vars, i))) {
>  		return FALSE;
>  	}
>  
> -	pseudo_type_info = MR_LIVE_TYPE_GET_VAR_TYPE(var->MR_slv_live_type);
> +	pseudo_type_info = MR_LIVE_TYPE_GET_VAR_TYPE(MR_var_pti(vars, i));

It would be better to write that as

	pseudo_type_info = MR_LIVE_TYPE_GET_VAR_TYPE(MR_var_pti(vars, i));
	if (!MR_LIVE_TYPE_IS_VAR(psuedo_type_info)) {
  		return FALSE;
  	}

> +++ mercury_stack_layout.h	1999/04/26 01:01:39
> +** (a) an integer with MR_LONG_LVAL_OFFSETBITS bits giving the index
> +** of the typeinfo inside a type class info (to be interpreted by
> +** private_builtin:type_info_from_typeclass_info), and (b) a MR_Long_Lval
> +** value giving the location of the pointer to the type class info.
> +** This MR_Long_Lval valud will *not* have an indirect tag.

s/valud/value/

> +#define	MR_SHORT_LVAL_STACKVAR(n)					\
> +	((MR_Short_Lval) (((n) << 2) + MR_SHORT_LVAL_TYPE_STACKVAR))
> +
> +#define	MR_SHORT_LVAL_FRAMEVAR(n)					\
> +	((MR_Short_Lval) (((n) << 2) + MR_SHORT_LVAL_TYPE_FRAMEVAR))
> +
> +#define	MR_SHORT_LVAL_R_REG(n)						\
> +	((MR_Short_Lval) (((n) << 2) + MR_SHORT_LVAL_TYPE_R))

The several occurrences of the number 2 should be replaced by a named
constant, e.g. MR_SHORT_LOCN_TAG_BITS.

> --- mercury_types.h	1998/12/15 00:22:29	1.14
> +++ mercury_types.h	1999/04/26 01:10:38
> @@ -26,10 +26,23 @@
>  ** this is ensured by the autoconfiguration script.
>  */
>  
> -typedef	unsigned WORD_TYPE	Word;
> -typedef WORD_TYPE		Integer;
> -typedef unsigned WORD_TYPE	Unsigned;
> -typedef WORD_TYPE		Bool;
> +#ifdef	HAVE_STDINT
> +#include <stdint.h>
> +#else
> +typedef	unsigned WORD_TYPE	uintptr_t;
> +typedef	WORD_TYPE		intptr_t;
> +typedef	unsigned INT32_TYPE	uint_least32_t;
> +typedef	INT32_TYPE		int_least32_t;
> +typedef	unsigned INT16_TYPE	uint_least16_t;
> +typedef	INT16_TYPE		int_least16_t;
> +typedef	unsigned char		uint_least8_t;
> +typedef	signed char		int_least8_t;
> +#endif

The stuff inside the `#if' (i.e. the `#include' and the typedefs)
should be intended two spaces.

> --- mercury_trace.c	1999/02/23 08:06:54	1.8
> @@ -425,12 +431,19 @@
>  		return 0;
>  	}
>  
> -	for (i = 0; i < label->MR_sll_var_count; i++) {
> +	for (i = 0; i < MR_all_desc_var_count(vars); i++) {
>  		if (streq(vars->MR_slvs_names[i], name)) {
> -			return MR_lookup_live_lval_base(
> -				vars->MR_slvs_pairs[i].MR_slv_locn, saved_regs,
> -				MR_saved_sp(saved_regs),
> +			if (i < MR_long_desc_var_count(vars)) {
> +				return MR_lookup_long_lval_base(
> +					MR_long_desc_var_locn(vars, i),
> +					saved_regs, MR_saved_sp(saved_regs),
> +					MR_saved_curfr(saved_regs), succeeded);
> +			} else {
> +				return MR_lookup_short_lval_base(
> +					MR_short_desc_var_locn(vars, i),
> +					saved_regs, MR_saved_sp(saved_regs),
>  				MR_saved_curfr(saved_regs), succeeded);
> +			}
>  		}
>  	}

I think it would be better to avoid duplicating the common code here,
and write that as

			Word locn;	/* or whatever type is appropriate */
			if (i < MR_long_desc_var_count(vars)) {
				locn = MR_long_desc_var_locn(vars, i);
			} else {
				locn = MR_short_desc_var_locn(vars, i);
			};
			return MR_lookup_live_lval_base(locn, saved_regs,
				MR_saved_sp(saved_regs),
  				MR_saved_curfr(saved_regs), succeeded);

> Index: trace/mercury_trace_external.c
> @@ -831,8 +831,7 @@
>  		** due to the fake arity of the type private_builtin:typeinfo/1.
>  		*/
>  		if ((strncmp(name, "TypeInfo", 8) == 0)
> -		    || (strncmp(name, "TypeClassInfo", 13) == 0)
> -		    ) {
> +		|| (strncmp(name, "TypeClassInfo", 13) == 0)) {
>  		  continue;
>  		}

Neither the old code nor the new code conform to our coding standards
for indentation.  The `{' should be on a line of its own, so it should
be

	if ((...)
	    || (...))
	{
		continue;
	}

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