[m-rev.] for post-commit reivew: Fix a bug in the dependant parallel conjunction transformation.

[Paul Bone]

For post-commit review by Peter Wang or Zoltan.
I'll commit this once the bootcheck passes (it's running now).

---

Fix a bug in the dependant parallel conjunction transformation.

The problem occurs when a wait is pushed into a callee and that callee consumes
the future only on some of its success paths.  The caller assumes that the
callee will always wait and that it is safe to use future_get() after the call
returns.  There seems to be an invariant or assumption that the callee will
wait if it returns successfully.  To preserve this invariant the following
patch ensures that a wait is added to the end of any procedure that didn't wait
on all of its success paths.

Zoltan has suggested that it may be more ideal to put the wait on the success
paths that don't consume the variable so that in any case a variable is waited
on at most once.  This is true unless there's a non-trivial call after the
branching goal, in this case the overlap between parallel conjunctions can be
improved by waiting at the end of the procedure.

compiler/dep_par_conj.m:
    As above.

Index: compiler/dep_par_conj.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/dep_par_conj.m,v
retrieving revision 1.42
diff -u -p -b -r1.42 dep_par_conj.m
--- compiler/dep_par_conj.m	11 Feb 2010 05:20:55 -0000	1.42
+++ compiler/dep_par_conj.m	25 Jul 2010 10:24:45 -0000
@@ -563,11 +563,18 @@ sync_dep_par_conjunct(ModuleInfo, AllowS
         ProducedVarsList = set.to_sorted_list(ProducedVars),
 
         % Insert waits into the conjunct, as late as possible.
-        list.map_foldl3(
-            insert_wait_in_goal(ModuleInfo, AllowSomePathsOnly,
+        (
+            ParConjunctStatus = par_conjunct_is_in_conjunction,
+            list.map_foldl3(insert_wait_in_goal(ModuleInfo, AllowSomePathsOnly,
                 FutureMap),
             ConsumedVarsList, _WaitedOnAllSuccessPaths,
-            !Goal, !VarSet, !VarTypes),
+                !Goal, !VarSet, !VarTypes)
+        ;
+            ParConjunctStatus = par_conjunct_is_proc_body,
+            list.foldl3(insert_wait_in_goal_for_proc(ModuleInfo,
+                    AllowSomePathsOnly, FutureMap),
+                ConsumedVarsList, !Goal, !VarSet, !VarTypes)
+        ),
 
         % Insert signals into the conjunct, as early as possible.
         list.foldl3(insert_signal_in_goal(ModuleInfo, FutureMap),
@@ -587,6 +594,26 @@ sync_dep_par_conjunct(ModuleInfo, AllowS
     ),
     update_instmap(!.Goal, !InstMap).
 
+:- pred insert_wait_in_goal_for_proc(module_info::in, bool::in, future_map::in,
+    prog_var::in, hlds_goal::in, hlds_goal::out,
+    prog_varset::in, prog_varset::out, vartypes::in, vartypes::out) is det.
+
+insert_wait_in_goal_for_proc(ModuleInfo, AllowSomePathsOnly, FutureMap,
+        ConsumedVar, !Goal, !VarSet, !VarTypes) :-
+    insert_wait_in_goal(ModuleInfo, AllowSomePathsOnly, FutureMap, 
+        ConsumedVar, WaitedOnAllSuccessPaths, !Goal, !VarSet, !VarTypes),
+    % If we did not wait on all success paths then we must insert a
+    % wait here.  This preserves the invariant that a procedure called
+    % with a future that it should wait on will wait on it in all
+    % cases.  This way any future_get calls after such a call are safe.
+    (
+        WaitedOnAllSuccessPaths = waited_on_all_success_paths
+    ;
+        WaitedOnAllSuccessPaths = not_waited_on_all_success_paths,
+        insert_wait_after_goal(ModuleInfo, FutureMap, ConsumedVar, !Goal,
+            !VarSet, !VarTypes)
+    ).
+
 %-----------------------------------------------------------------------------%
 
 :- type waited_on_all_success_paths
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 489 bytes
Desc: Digital signature
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20100725/55046e17/attachment.sig>