[m-rev.] for post-commit review: push goals in implicit_parallelism.m

[Zoltan Somogyi]

On 05-Jan-2011, Paul Bone <pbone at csse.unimelb.edu.au> wrote:
> > +    % failed. This can happen because the program has changed since PushGoals
> > +    % was computed and put into the feedback file, or because PushGoals is
> > +    % inconsistent (regardless of the date of the file). One example of an
> > +    % inconsistency would be asking to push a goal into the condition of an
> > +    % if-then-else.
> > +    %
> 
> With regards to pushing goals into the condition of an ITE.  That's not really
> an inconsistency between the HLDS and the feedback data.  It's more or an error
> in the feedback data since a push into a semidet conjunction is not part of our
> design (at this stage).

I reworded the comment.

> > +    proc_info_get_rtti_varmaps(!.ProcInfo, RttiVarMaps0),
> > +    PushInfo = push_info(RttiVarMaps0),
> > +    do_push_list(PushGoals, PushInfo, OverallResult, Goal0, Goal1),
> > +    (
> > +        OverallResult = push_failed
> 
> So if one push fails we throw away Goal1 thereby rejecting all of the pushes.
> We may still be able to keep Goal1 and apply some of the automatic
> parallelisations (but probably not all of them).

For now, I expect the average number of pushes per predicate to be 1.00001,
so it does not really matter.

> We should also emit an error or warning, - I'm happy to implement this.

It is not urgent.

> As above, we should probably try the remaining pushes incase they allow some
> more parallelisation.

It is trivial to do the remaining pushes, but not as trivial to figure out
which parallelisation transformations planned by the tool are still valid,
or how their goal paths need to be adjusted. Do after the paper deadline.

> Somewhere (maybe goal_util.m) there is a predicate for performing a
> transformation on a goal at a given goal path.  This can be used navigate to
> that goal, do the transformation and as the call stack unwinds it rebuilds the
> HLDS.

I did not remember that. I will look into it.

> > +        % If PushConjNum specifies a conjunct that is NOT the last conjunct
> > +        % before Lo, then this transformation reorders the code.
> > +        % For now, we don't allow that.
> 
> I will add a comment here that says this can be avoided by ensuring that the
> mdprof_Fb tool pushes as many goals as necessary such that this constraint holds.
> This is Lo (for now) will always be PushGOalNum + 1.

Isn't that obvious?

> > +is_pushable_goal(PushInfo, Goal, Pushable) :-
> > +    Goal = hlds_goal(GoalExpr, GoalInfo),
> > +    Purity = goal_info_get_purity(GoalInfo),
> > +    Detism = goal_info_get_determinism(GoalInfo),
> > +    (
> > +        Purity = purity_pure,
> > +        Detism = detism_det
> 
> How about cc_mutli?

I haven't thought that through yet, and I don't think there is need for it,
at least not yet.

> > +is_non_rtti_var(RttiVarMaps, Arg) :-
> > +    rtti_varmaps_var_info(RttiVarMaps, Arg, RttiVarInfo),
> > +    RttiVarInfo = non_rtti_var.
> 
> I don't know enough about our RTTI vars to answer this for myself.  If a
> deconstruction can refer to an RTTI var can any other goal such as a call?  Or
> at least why is it a problem for duplication of deconstructions and not for
> other goals?

I added a comment linking to the reason.

> > +                % If ConjNum specifies a conjunct that is NOT the last
> > +                % conjunct, then this transformation reorders the code.
> > +                % For now, we don't allow that.
> > +                list.length(Conjuncts0, Length),
> > +                ConjNum = Length
> 
> This can also be worked around by picking up and moving all the conjuncts after
> the one that we're pusing into and prepending them to LoHi creating a new LoHi.

It could be. If it turn out to be a problem, I will change the code.
In the meantime, I added a comment.

> Allowing re-ordering is probably better in many cases.

Testing whether that is safe is easy. Testing whether it is a good idea
is probably significantly harder.

> > +            GoalExpr0 = scope(Reason, SubGoal0),
> > +            SubGoal0 = hlds_goal(_SubGoalExpr0, SubGoalInfo0),
> > +            Detism = goal_info_get_determinism(GoalInfo0),
> > +            SubDetism = goal_info_get_determinism(SubGoalInfo0),
> > +            (
> > +                Detism = SubDetism,
> > +                maybe_steps_after(step_neg, HeadSteps, HeadStepsAfter),
> > +                list.map(maybe_steps_after(step_neg), TailSteps,
> > +                    TailStepsAfter)
> 
> Is step_neg here a mistake?

Yes. Thanks for the catch.

Zoltan.

cvs diff: Diffing .
Index: push_goals_together.m
===================================================================
RCS file: /home/mercury/mercury1/repository/mercury/compiler/push_goals_together.m,v
retrieving revision 1.1
diff -u -b -r1.1 push_goals_together.m
--- push_goals_together.m	4 Jan 2011 05:31:38 -0000	1.1
+++ push_goals_together.m	5 Jan 2011 04:12:52 -0000
@@ -38,8 +38,8 @@
     % push_succeeded if they all worked, and push_failed if at least one
     % failed. This can happen because the program has changed since PushGoals
     % was computed and put into the feedback file, or because PushGoals is
-    % inconsistent (regardless of the date of the file). One example of an
-    % inconsistency would be asking to push a goal into the condition of an
+    % invalid (regardless of the date of the file). One example of a validity
+    % problem would be asking to push a goal into the condition of an
     % if-then-else.
     %
 :- pred push_goals_in_proc(list(push_goal)::in, push_result::out,
@@ -404,6 +404,8 @@
             ;
                 Unification = deconstruct(_, _, Args, _, _, _),
                 RttiVarMaps = PushInfo ^ pi_rtti_varmaps,
+                % See the comment in move_follow_code_select in follow_code.m 
+                % for the reason for this test.
                 ( list.all_true(is_non_rtti_var(RttiVarMaps), Args) ->
                     Pushable = pushable
                 ;
@@ -583,6 +585,10 @@
                 % If ConjNum specifies a conjunct that is NOT the last
                 % conjunct, then this transformation reorders the code.
                 % For now, we don't allow that.
+                %
+                % However, we could also avoid reordering by removing
+                % all the conjuncts after ConjNum from this conjunction,
+                % and moving them to the front of LoHi.
                 list.length(Conjuncts0, Length),
                 ConjNum = Length
             ->
@@ -676,8 +682,9 @@
             SubDetism = goal_info_get_determinism(SubGoalInfo0),
             (
                 Detism = SubDetism,
-                maybe_steps_after(step_neg, HeadSteps, HeadStepsAfter),
-                list.map(maybe_steps_after(step_neg), TailSteps,
+                maybe_steps_after(step_scope(scope_is_no_cut)), HeadSteps,
+                    HeadStepsAfter),
+                list.map(maybe_steps_after(scope_is_no_cut)), TailSteps,
                     TailStepsAfter)
             ->
                 push_into_goal(LoHi, HeadStepsAfter, TailStepsAfter,
cvs diff: Diffing notes
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------