[m-dev.] Re: for review: minimal model tabling

[Zoltan Somogyi]

I made all the trivial changes Fergus requested. Here are my responses
to the nontrivial ones.

> A comment here explaining why we need to call MR_commit_mark()
> if --use-minimal-model is set would be helpful.

% If the code we are committing across starts but
% does not complete the evaluation of a tabled subgoal,
% the cut will remove the generator's choice point,
% so that the evaluation of the subgoal will never
% be completed. We handle such "dangling" generators
% by removing them from the subgoal trie of the
% tabled procedure. This requires knowing what
% tabled subgoals are started inside commits,
% which is why we wrap the goal being committed across
% inside MR_commit_{mark,cut}.

> > +	if (c->cutstack_zone != NULL) {
> > +		reset_redzone(c->cutstack_zone);
> > +	} else {
> > +		c->cutstack_zone = create_zone("cutstack", 0,
> > +			cutstack_size, next_offset(),
> > +			cutstack_zone_size, default_handler);
> > +	}
> > +	c->context_cut_sp = 0;
> 
> I think all that code should be inside "#ifdef MR_USE_MINIMAL_MODEL"
> or something like that.
> 
> Likewise for all the other new code added to mercury_context.h,
> mercury_context.c, mercury_memory.c, mercury_stacks.c,
> and mercury_tabling.c.  Also for the code in
> trace/mercury_trace_internal.h.

OK, since I see your point that the new stuff is not necessary
only if you use minimal model tabling. However, the price of this is that
people who don't enable -DMR_USE_MINIMAL_MODEL and --use-minimal-model
will not be warned by bootcheck if their changes break minimal model tabling,
since the test cases for minimal model will have to be disabled then.

> > +#include "mercury_regs.h"	/* must be before prototypes */
> 
> I don't understand the comment here.  Why must "mercury_regs.h"
> be #included before the function prototypes?

That is documented in mercury_regs.h itself.

> > +save_state(MR_SavedState *saved_state,
> > +	Word *generator_maxfr, Word *generator_sp,
> > +	const char *who, const char *what)
> 
> A comment explaining what this function is supposed to do
> would be helpful.

/*
** Save the current state of the Mercury abstract machine, so that the
** current computation may be suspended for a while, and restored later.
** The generator_{maxfr,sp} arguments give the points from which we need
** to copy the nondet and the det stacks. The parts of those stacks below
** the given points will not change between the suspension and the resumption
** of this state, or if they do, the stack segments in the saved state
** will be extended (via extend_consumer_stacks).
*/

> > +	table_copy_bytes(saved_state->generator_stack_block,
> > +		(char *) MR_gen_stack,
> > +		MR_gen_sp * sizeof(MR_GeneratorStackFrame));
> 
> If MR_gen_sp is a pointer, then the above code is buggy.
> If MR_gen_sp is actually an array index, rather than a pointer,
> then I think perhaps `MR_gen_sp' is a misleading name.

It is an index, but it still "points" to a stack slot.

I have renamed MR_gen_sp as MR_gen_next, and similarly for MR_cut_sp.

> Why the cast to `char *' here?

Because MR_gen_stack is a pointer to an array of generator stack frame
structures, not chars.

> > +	table_copy_bytes(saved_state->cut_stack_block,
> > +		(char *) MR_cut_stack,
> > +		(MR_cut_sp + 1) * sizeof(MR_CutStackFrame));
> 
> Why the cast to `char *' here?

Similar reason.

> > +static void
> > +extend_consumer_stacks(MR_Subgoal *leader, MR_Consumer *suspension)
> 
> A comment explaining what this function is supposed to do would be
> helpful.

/*
** The saved state of a consumer for a subgoal (say subgoal A) includes
** the stack segments between the tops of the stack at the time that
** A's generator was entered and the time that A's consumer was entered.
** When A becomes a follower of another subgoal B, the responsibility for
** scheduling A's consumers passes to B's generator. Since by definition
** B's nondet stack frame is lower in the stack than A's generator's,
** we need to extend the stack segments of A's consumers to also include
** the parts of the stacks between the generator of B and the generator of A.
*/

> > +static void
> > +make_subgoal_follow_leader(MR_Subgoal *this_follower, MR_Subgoal *leader)
> 
> Likewise here.

/*
** When we discover that two subgoals depend on each other, neither can be
** completed alone. We therefore pass responsibility for completing all
** the subgoals in an SCC to the subgoal whose nondet stack frame is
** lowest in the nondet stack.
*/

> > +/* virtual machine registers that we don't even try to make real ones */
> > +Integer			MR_gen_sp = 0;
> > +Integer			MR_cut_sp = 0;
> > +MR_GeneratorStackFrame	*MR_gen_stack = NULL;
> > +MR_CutStackFrame	*MR_cut_stack = NULL;
> 
> Hmm, shouldn't they go in the MR_Engine data structure?

They *are* in the MR_Engine data structure (specifically, the MR_Context part).
That is where they live in non-active threads. These global vars are for the
active thread. (maxfr, curfr, sp etc are not in MR_Engine either.) The thread
activation/suspension routines handle the rest.

> If tabling is not supposed to work in conjunction with multithreading,
> then that should be documented, with appropriate XXX comments,
> calls to fatal_error("sorry, not implemented: ...") and/or
> compile-time `#ifdef ... #error ... #endif' tests.

OK.

> > :- type ml_table == c_pointer.
> > :- type ml_subgoal_table_node == c_pointer.
> > :- type ml_answer_table_node == c_pointer.
> > :- type ml_answer_slot == c_pointer.
> > :- type ml_answer_block == c_pointer.
> 
> I think it would be clearer to write those as
> 
> 	:- type ml_subgoal_table_node == ml_table.
> 	:- type ml_answer_table_node == ml_table.
> 	:- type ml_answer_slot == ml_table.
> 	:- type ml_answer_block == ml_table.

OK.

> And is there any reason why the `type ml_table == c_pointer' equivalence
> is exported?

No. Only the fact that ml_* are equivalent to ml_table needs to be.

> > :- pragma c_code("
> > BEGIN_MODULE(private_builtin_module_XXX)
> 
> What's the XXX for?

The comment just above the stuff you quote explains that.

Zoltan.