[m-rev.] for review: extending I/O tabling towards declarative debugging

[Mark Brown]

On 18-Jan-2002, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Index: compiler/hlds_data.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/hlds_data.m,v
> retrieving revision 1.63
> diff -u -b -r1.63 hlds_data.m
> --- compiler/hlds_data.m	2002/01/16 01:13:18	1.63
> +++ compiler/hlds_data.m	2002/01/17 11:52:39
> @@ -55,7 +55,11 @@
>  				% that points to the table that implements
>  				% memoization, loop checking or the minimal
>  				% model semantics for the given procedure.
> -			;	deep_profiling_proc_static(rtti_proc_label).
> +			;	deep_profiling_proc_static(rtti_proc_label)
> +				% The address of a procedure's layout
> +				% structure, used e.g. for I/O tabling for
> +				% declarative debugging.
> +			;	table_io_decl(rtti_proc_label).

The comment here should go below the new constructor.  Note that the
existing 'deep_profiling_proc_static' constructor has no comment
attached to it.

Also, why do you say this is the address of the procedure's layout?
It sounds like it refers to an MR_Proc_Layout, but doesn't it refer to
an MR_Table_Io_Decl structure?  (See also the comment for unify_gen.m,
below.)

> Index: compiler/stack_layout.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/stack_layout.m,v
> retrieving revision 1.60
> diff -u -b -r1.60 stack_layout.m
> --- compiler/stack_layout.m	2001/01/29 06:47:20	1.60
> +++ compiler/stack_layout.m	2001/12/28 11:10:28
> @@ -1043,6 +1064,65 @@
>  	ll_pseudo_type_info__construct_typed_llds_pseudo_type_info(Type,
>  		NumUnivQTvars, ExistQTvars, ArgRval, ArgRvalType, C0, C).
>  
> +:- pred stack_layout__add_one(pair(maybe(rval), llds_type)::in,
> +	pair(int, maybe(llds_type))::out) is det.
> +
> +stack_layout__add_one(_MaybeRval - LldsType, 1 - yes(LldsType)).
> +
> +%---------------------------------------------------------------------------%
> +
> +:- pred stack_layout__make_table_decl_io_data(rtti_proc_label::in,
> +	proc_layout_kind::in, table_io_decl_info::in, layout_data::out,
> +	stack_layout_info::in, stack_layout_info::out) is det.

This predicate should be named make_table_io_decl_data.

> @@ -1380,6 +1460,7 @@
>  		trace_level		:: trace_level,
>  		trace_suppress_items	:: trace_suppress_items,
>  		static_code_addresses	:: bool, % have static code addresses?
> +		table_io_decl		:: list(comp_gen_c_data),
>  		proc_layouts		:: list(comp_gen_c_data),
>  		internal_layouts	:: list(comp_gen_c_data),
>  		label_set		:: map(label, data_addr),

Since there is a list of them, a better name for this field would be
table_io_decls.

> Index: compiler/table_gen.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/table_gen.m,v
> retrieving revision 1.30
> diff -u -b -r1.30 table_gen.m
> --- compiler/table_gen.m	2001/11/26 09:30:58	1.30
> +++ compiler/table_gen.m	2002/01/14 14:28:44
> @@ -430,23 +462,59 @@
>  		semidet, yes(impure), [TableVar0 - ground(shared, none),
>  		CounterVar - ground(shared, none),
>  		StartVar - ground(shared, none)],
> -		Module, Context, InRangeGoal),
> +		ModuleInfo, Context, InRangeGoal),
>  
>  	generate_new_table_var("TableVar", VarTypes3, VarTypes4,
>  		VarSet3, VarSet4, TableVar),
>  	generate_call("table_lookup_insert_start_int",
>  		[TableVar0, StartVar, CounterVar, TableVar],
>  		det, yes(impure), [TableVar - ground(unique, none)],
> -		Module, Context, LookupGoal),
> +		ModuleInfo, Context, LookupGoal),
>  
>  	generate_call("table_io_has_occurred", [TableVar],
> -		semidet, yes(semipure), [], Module, Context, OccurredGoal),
> +		semidet, yes(semipure), [], ModuleInfo, Context, OccurredGoal),
>  
> -	generate_restore_goal(SavedOutputVars, TableVar, Module, Context,
> -		VarTypes4, VarTypes6, VarSet4, VarSet6, RestoreAnsGoal0),
> +	(
> +		TableDecl = yes,
> +		PredId = TableInfo0 ^ table_cur_pred_id,
> +		ProcId = TableInfo0 ^ table_cur_proc_id,
> +		RttiProcLabel = rtti__make_proc_label(ModuleInfo,
> +			PredId, ProcId),
> +		TableIoDeclConsId = table_io_decl(RttiProcLabel),
> +		get_table_var_type(TableVarType),
> +		make_const_construction(TableIoDeclConsId, TableVarType,
> +			yes("TableIoDeclPtr"), TableIoDeclGoal,
> +			TableIoDeclPtrVar, VarTypes4, VarTypes5,
> +			VarSet4, VarSet5),
> +		allocate_slot_numbers(SavedHeadVars, 1, NumberedSavedHeadVars),
> +		NumberedSaveVars = [TableIoDeclPtrVar - 0 |
> +			NumberedSavedHeadVars],
> +		list__filter(key_belong_to_list(SavedOutputVars),
> +			NumberedSaveVars, NumberedSavedOutputVars),
> +		NumberedRestoreVars = NumberedSavedOutputVars,
> +		list__length(NumberedSaveVars, BlockSize),
> +
> +		ProcInfo0 = TableInfo0 ^ table_cur_proc_info,
> +		continuation_info__generate_table_decl_io_layout(ProcInfo0,
> +			NumberedSavedHeadVars, TableIoDeclInfo),
> +		MaybeTableIoDeclInfo = yes(TableIoDeclInfo)
> +	;
> +		TableDecl = no,
> +		VarTypes5 = VarTypes4,
> +		VarSet5 = VarSet4,
> +		true_goal(TableIoDeclGoal),
> +		allocate_slot_numbers(SavedOutputVars, 0,
> +			NumberedSavedOutputVars),
> +		NumberedRestoreVars = NumberedSavedOutputVars,
> +		NumberedSaveVars = NumberedSavedOutputVars,
> +		list__length(NumberedSavedOutputVars, BlockSize),
> +		MaybeTableIoDeclInfo = no
> +	),

The block size is always going to be the length of the saved vars list,
irrespective of whether they are output or not.  So it would be a bit
clearer, IMHO, to calculate the block size outside this switch, ie:

	list__length(NumberedSaveVars, BlockSize),

> Index: compiler/unify_gen.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/unify_gen.m,v
> retrieving revision 1.117
> diff -u -b -r1.117 unify_gen.m
> --- compiler/unify_gen.m	2001/10/31 16:58:10	1.117
> +++ compiler/unify_gen.m	2001/12/25 10:46:23
> @@ -254,6 +254,9 @@
>  unify_gen__generate_tag_test_rval_2(deep_profiling_proc_static_tag(_), _, _) :-
>  	% This should never happen
>  	error("Attempted deep_profiling_proc_static_tag unification").
> +unify_gen__generate_tag_test_rval_2(table_io_decl_tag(_), _, _) :-
> +	% This should never happen
> +	error("Attempted table_io_decl_tag unification").
>  unify_gen__generate_tag_test_rval_2(no_tag, _Rval, TestRval) :-
>  	TestRval = const(true).
>  unify_gen__generate_tag_test_rval_2(single_functor, _Rval, TestRval) :-
> @@ -418,6 +421,15 @@
>  	),
>  	{ DataAddr = layout_addr(proc_static(RttiProcLabel)) },
>  	code_info__assign_const_to_var(Var, const(data_addr_const(DataAddr))).
> +unify_gen__generate_construction_2(table_io_decl_tag(RttiProcLabel),
> +		Var, Args, _Modes, _, _, empty) -->
> +	( { Args = [] } ->
> +		[]
> +	;
> +		{ error("unify_gen: proc_layout has args") }

Why proc_layout?  Isn't this a table_io_decl?

> Index: runtime/mercury_stack_layout.h
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/runtime/mercury_stack_layout.h,v
> retrieving revision 1.53
> diff -u -b -r1.53 mercury_stack_layout.h
> --- runtime/mercury_stack_layout.h	2001/12/10 06:50:11	1.53
> +++ runtime/mercury_stack_layout.h	2002/01/14 04:43:27
> @@ -414,6 +415,29 @@
>  */
>  
>  /*
> +** The MR_Table_Io_Decl structure.
> +**
> +** To enable declarative debugging of I/O actions, the compiler generates one
> +** of these structures for each I/O primitive. The compiler transforms the
> +** bodies of those primitives to create a block of memory and fill it in with
> +** (a) a pointer to the primitive's MR_Table_Io_Decl structure and (2) the
> +** values of the primitive's arguments (both input and output, but excluding
> +** the I/O states). The array of pseudo-typeinfos pointed to by the ptis field
> +** gives the types of these arguments, while the arity field gives the number
> +** of arguments (and thus the size of the ptis array, and the size of the block
> +** exclusive of the pointer). The proc field allows us to identify the
> +** primitive procedure. This is all the information we need to describe the
> +** I/O action to the user.
> +*/
> +
> +typedef struct MR_Table_Io_Decl_Struct {
> +	const MR_Proc_Layout		*MR_table_io_decl_proc;
> +	MR_Integer			MR_table_io_decl_arity;

*_arity is a bit misleading, since it is not necessarily equal to the
procedure's arity.  I think naming this field MR_table_io_decl_block_size
would be a big improvement.

This completes this round of reviewing.

Cheers,
Mark.

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