[m-rev.] For review: Implemention of the region runtime system
Quan Phan
quan.phan at cs.kuleuven.be
Tue Oct 9 04:24:55 AEST 2007
On Mon, Oct 08, 2007 at 06:48:45PM +1000, Zoltan Somogyi wrote:
>
> I agree with Julien's comments, as far as they go, but I have my own as well.
>
> cvs diff: Diffing compiler
> Index: compiler/code_info.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/code_info.m,v
> retrieving revision 1.350
> diff -u -r1.350 code_info.m
> --- compiler/code_info.m 1 Oct 2007 04:06:47 -0000 1.350
> +++ compiler/code_info.m 5 Oct 2007 22:50:34 -0000
> -maybe_save_region_commit_frame(AddRegionOps, ForwardLiveVarsBeforeGoal,
> - MaybeRegionCommitFrameInfo, Code, !CI) :-
> +maybe_save_region_commit_frame(AddRegionOps, _ForwardLiveVarsBeforeGoal,
> + CommitGoalInfo, MaybeRegionCommitFrameInfo, Code, !CI) :-
> (
> AddRegionOps = do_not_add_region_ops,
> MaybeRegionCommitFrameInfo = no,
> Code = empty
> ;
> AddRegionOps = add_region_ops,
> + MaybeRbmmInfo = goal_info_get_maybe_rbmm(CommitGoalInfo),
> + (
> + MaybeRbmmInfo = no,
> + MaybeRegionCommitFrameInfo = no,
>
> Is this possible if AddRegionOps = add_region_ops? If it is possible, under
> what circumstances can it happen?
I think no. The intention of rbmm.add_rbmm_goal_infos.m is to record the
rbmm_goal_info field even when there is no information, i.e.,
initializes the term with all the sets are empty. So after the pass,
calls to goal_info_get_rbmm should succeed.
The true story here is that when I used goal_info_get_rbmm, I got an
error that RBMM info is not available (from goal_info_get_rbmm
predicate). I could not explain it at that time so I switched to
goal_info_get_maybe_rbmm. I should have put a XXX comment for it.
>
> Why do you call the extra variable CommitGoalInfo? It is the GoalInfo of the
> goal *inside* the commit, not of the commit goal itself.
Yes, it's my mistake. It should probably be changed to InsideCommitGoalInfo.
>
> Index: compiler/disj_gen.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/disj_gen.m,v
> retrieving revision 1.105
> diff -u -r1.105 disj_gen.m
> --- compiler/disj_gen.m 27 Sep 2007 10:42:05 -0000 1.105
> +++ compiler/disj_gen.m 5 Oct 2007 22:50:35 -0000
> @@ -386,16 +386,38 @@
> % XXX The condition should succeed only if some disjunct performs
> % some region operations (allocation or removal). The HLDS does not yet
> % contain the information we need to decide whether this is the case.
>
> You should delete this now-obsolete XXX.
I will. There are a few of them, which I leave there just to help
reminding of what is there before my changes. If the changes are correct
we can delete the obsolete comments or modify them to suit with the changes.
>
> + MaybeRbmmInfo = goal_info_get_maybe_rbmm(DisjGoalInfo),
> + (
> + MaybeRbmmInfo = no,
>
> Can I presume that this means we are not in an rbmm grade?
This comes from the same problem above.
>
> + ( if
> + set.empty(DisjCreatedRegionVars),
> + set.empty(DisjRemovedRegionVars),
> + set.empty(DisjAllocRegionVars)
> + then
> + FirstRegionCode = empty,
> + LaterRegionCode = empty,
> + LastRegionCode = empty,
> + RegionStackVarsToRelease = []
> + else
> + % We only need region support for backtracking if
> + % some disjunct performs some region operations
> + % (allocation or removal)
> + maybe_create_disj_region_frame(AddRegionOps, DisjGoalInfo,
> + FirstRegionCode, LaterRegionCode, LastRegionCode,
> + RegionStackVars, !CI),
> + RegionStackVarsToRelease = RegionStackVars
> + )
>
> This module uses (C -> T ; E) style if-then-elses.
Ok, it will be changed.
>
> @@ -445,7 +467,7 @@
> unexpected(this_file, "generate_disjuncts: empty disjunction!").
> generate_disjuncts([Goal0 | Goals], CodeModel, FullResumeMap,
> MaybeEntryResumePoint, HijackInfo, DisjGoalInfo, EndLabel, ReclaimHeap,
> - MaybeHpSlot0, MaybeTicketSlot, LaterRegionCode, LastRegionCode,
> + MaybeHpSlot0, MaybeTicketSlot, LaterRegionCode0, LastRegionCode,
> BranchStart0, MaybeEnd0, MaybeEnd, Code, !CI) :-
>
> Your naming scheme (LaterRegionCode0, LaterRegionCode) implies that one data
> structure is being updated, which is not the case. A better scheme would be to
> keep the old name LaterRegionCode, and to use a new name such as
> ThisDisjRegionCode for the variable that is set to empty for the first disjunct
> and to LaterRegionCode for the non-first & non-last disjuncts.
>
Ok, it will be changed.
> yes(NextResumePoint), HijackInfo, DisjGoalInfo,
> EndLabel, ReclaimHeap, MaybeHpSlot, MaybeTicketSlot,
> - LaterRegionCode, LastRegionCode, BranchStart,
> + LaterRegionCode0, LastRegionCode, BranchStart,
> MaybeEnd1, MaybeEnd, RestCode, !CI),
>
> Code = tree_list([
> @@ -654,6 +678,10 @@
> % variables that are statically known to be already protected by
> % an outer disjunction, but we don't yet have the program analysis
> % required to gather such information.
> + % XXX Protecting only forward live region variables is not enough,
> + % therefore not correct. We need to protect backward live region
> + % variables also. How can we find this information? It seems that we
> + % need to run liveness.m first.
>
> I don't understand this comment. Protecting any region that is not
> forward live would be redundant, since there should not be any remove
> operations for them.
>
Please ignore this XXX comment. The root of it is because I had a
problem (bug) that a region which should be protected is not protected by a
disj frame when running the crypt benchmark. It is likely that the
ProtectRegionVars (in disj_gen.m) does not contain some region variables
that it should. One thing I've noticed is in commit_gen.m we use
ForwardLiveVarsBeforeGoal (this is correct), not ForwardLiveVars, which
are after the goal. Why is the difference? The same thing happens in
ite_gen.m.
Zoltan, do you have a running rbmm system yet? To compile crypt.m, I
need to use --no-inlining, otherwise the region analysis falls into an
infinite loop. If you can run crypt and reproduce the error, we can
discuss this problem in another email.
> Index: compiler/ite_gen.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/ite_gen.m,v
> retrieving revision 1.102
> diff -u -r1.102 ite_gen.m
> --- compiler/ite_gen.m 1 Oct 2007 04:06:48 -0000 1.102
> +++ compiler/ite_gen.m 5 Oct 2007 22:50:35 -0000
> @@ -559,144 +559,163 @@
> get_forward_live_vars(!.CI, ForwardLiveVars),
> LiveRegionVars = filter_region_vars(!.CI, ForwardLiveVars),
>
> + MaybeRbmmInfo = yes(RbmmInfo),
> + RbmmInfo = rbmm_goal_info(CondCreatedRegionVars,
> + CondRemovedRegionVars, CondCarriedRegionVars,
> + CondAllocRegionVars, _CondUsedRegionVars),
> + ( ( set.empty(CondCreatedRegionVars),
> + set.empty(CondRemovedRegionVars),
> + set.empty(CondAllocRegionVars)
> + ) ->
>
> Please fix the indentation. It should look like this:
>
> (
> set.empty(CondCreatedRegionVars),
> set.empty(CondRemovedRegionVars),
> set.empty(CondAllocRegionVars)
> ->
> ...
>
Ok, it will be fixed.
> + % The UnprotectedRemovedAtStartOfElse is the
> + % intersection of RemovedAtStartOfElse and the set of region
> + % variables whose regions are statically known to be
> + % unprotected at this point in the code. These are actually
> + % carried regions because carried region are statically known
> + % to be not protected by the condition.
>
> I don't understand the text you added here. Do you mean
>
> (a) unprotected here implies carried in the condition
> (b) unprotected here is implied by carried in the condition
> (c) both (a) and (b)
The first sentence is yours. I think that "regions statically known to be
unprotected" are carried ones, so it is (c). Is it what you meant?
>
> Index: compiler/options.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/options.m,v
> retrieving revision 1.587
> diff -u -r1.587 options.m
> --- compiler/options.m 4 Oct 2007 09:04:43 -0000 1.587
> +++ compiler/options.m 5 Oct 2007 22:50:39 -0000
> @@ -1204,7 +1204,7 @@
> size_region_commit_fixed - int(3),
> size_region_ite_protect - int(1),
> size_region_ite_snapshot - int(4),
> - size_region_disj_protect - int(2),
> + size_region_disj_protect - int(1),
> size_region_disj_snapshot - int(4),
> size_region_commit_entry - int(1)
> ]).
>
> You probably want to add a comment here pointing to the corresponding #defines
> in runtime/mercury_region.h.
Yes, not only the corresponding #defines, but also the corresponding
type structs if we will use any of them. The effort to keep the comment
consistent seems worth it because I once got a bug due to the change of
size_region_disj_protect from 2 to 1 and I forgot to delete a field in
MR_Disj_Protection struct. The bug is not easy to detect.
>
> The code in the library cannot be compiled with the runtime as shown in the
> diff, because you (a) call the function for printing region profiling data
> in all grades, but (b) define that function only in rbmm grades.
>
> The code in the runtime also has lots of other problems:
>
> - it uses C++ extensions, like // comments, and declarations after code,
> that aren't allowed in strict C compilers
>
> - it doesn't have MR_ prefixes on the names of all functions and structure
> fields
>
> - it departs in several respects from our code style conventions
>
> - runtime/mercury_region.h wasn't being included in any file except
> library/region_builtin.m
>
> Pointing out each place that should be fixed would have been more work
> than doing the fixes, so I have prepared an updated diff. I am testing it now,
> but a proper test (in both asm_fast.gc and asm_fast.gc.rbmm grades, and both
> with and without region debugging and profiling enabled) will unfortunately
> require a day or so.
>
> The provisional diff (which is attached) still leaves some structure field
> names unqualified, and still uses address arithmetic to access the slots
> of embedded stack frames (I expect you could define a structure for the fixed
> slots, and access them by name), but I want to test and debug the change
> so far before I tackle those.
>
Because you also produce a diff, i.e., making changes here and there,
could you also make the small changes you mentioned above (variable
names, ite style, etc)? Or should I make the changes in the *.m files
and commit them before the C files?
Regards,
Quan
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to: mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions: mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------
More information about the reviews
mailing list