[m-rev.] diff: cleanup of par_lopo_control.m

[Paul Bone]

On Tue, Oct 18, 2011 at 12:58:29PM +1100, Zoltan Somogyi wrote:
> compiler/par_loop_control.m:
> 	Simplify some code. Fix some comments. Give some predicates
> 	more meaningful names.
> 

I know this wasn't for review, but you asked me verbally if it was okay, so
I've made some comments below.

Thanks.

> Index: compiler/par_loop_control.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/par_loop_control.m,v
> retrieving revision 1.3
> diff -u -b -r1.3 par_loop_control.m
> --- compiler/par_loop_control.m	9 Oct 2011 09:56:20 -0000	1.3
> +++ compiler/par_loop_control.m	17 Oct 2011 13:35:28 -0000
> @@ -782,57 +790,61 @@
>      hlds_goal_info::in, hlds_goal::out, prog_varset::in, prog_varset::out,
>      vartypes::in, vartypes::out) is det.
>  
> -par_conj_loop_control(_, [], _, _, !VarSet, !VarTypes) :-
> -    unexpected($module, $pred, "empty parallel conjunction").
> -par_conj_loop_control(Info, [LastConj0 | RevConjs], GoalInfo, Goal, !VarSet,
> +par_conj_loop_control(Info, Conjuncts0, GoalInfo, Goal, !VarSet,
>          !VarTypes) :-
> +    list.det_split_last(Conjuncts0, EarlierConjuncts0, LastConjunct0),
>      % Re-write the recursive call in the last conjunct.
> -    goal_rewrite_recursive_call(Info, LastConj0, LastConj, _),
> -    goal_to_conj_list(LastConj, LastConjGoals),
> +    goal_rewrite_recursive_call(Info, LastConjunct0, LastConjunct, _),
> +    goal_to_conj_list(LastConjunct, LastConjGoals),
>  
>      % Process the remaining conjuncts.
> -    par_conj_loop_control2(Info, RevConjs, LastConjGoals, Goals, !VarSet,
> -        !VarTypes),
> -    create_conj_from_list(Goals, plain_conj, Goal0),
> +    rewrite_nonrecursive_par_conjuncts(Info,
> +        EarlierConjuncts0, EarlierConjuncts, !VarSet, !VarTypes),
> +    Conjuncts = EarlierConjuncts ++ LastConjGoals,
> +    % XXX The point of calling create_conj_from_list is that it sets up
> +    % the goal_info of Goal0 appropriately. Why call it if we then immediately
> +    % overwrite the goal_info?
> +    create_conj_from_list(Conjuncts, plain_conj, Goal0),
>      Goal1 = Goal0 ^ hlds_goal_info := GoalInfo,
>      fixup_goal_info(Info, Goal1, Goal).

I didn't see how it could set the GoalInfo correctly, for example it doesn't
know which variables are local to the conjunction and therefore that they
shouldn't appear in the nonlocals set.  Am I mistaken?

>      % Process each of the conjuncts in reverse order, building the new
>      % expression from them.
>      %

This now works in forwards-order.  I'll update the comment.

> @@ -951,18 +961,20 @@
>      %     conjunctions so that they call the inner procedure and pass the loop
>      %     control variable.
>      %
> -:- pred goal_loop_control_fixup(loop_control_info::in,
> +:- pred goal_update_non_loop_control_paths(loop_control_info::in,
>      list(goal_id)::in, fixup_goal_info::out,
>      hlds_goal::in, hlds_goal::out) is det.
>  
> -goal_loop_control_fixup(Info, RecParConjIds, FixupGoalInfo, !Goal) :-
> +goal_update_non_loop_control_paths(Info, RecParConjIds, FixupGoalInfo,
> +        !Goal) :-
>      GoalInfo0 = !.Goal ^ hlds_goal_info,
>      GoalId = goal_info_get_goal_id(GoalInfo0),
>      (
> -        % This goal is one of the trasnformed parallel conjunctions, nothing
> -        % needs to be done.
> -        % XXX: This may not work, I don't know if the goal ID is maintained.
> -        member(GoalId, RecParConjIds)
> +        % This goal is one of the transformed parallel conjunctions,
> +        % nothing needs to be done.
> +        % XXX What if the last conjunct contains a base case?

I've now checked, merge_loop_control_par_conjs_between_branches rejects cases
where this may happen.  I've adjusted the comment.

> +        % XXX This may not work, I don't know if the goal ID is maintained.
> +        list.member(GoalId, RecParConjIds)
>      ->
>          FixupGoalInfo = do_not_fixup_goal_info
>      ;
> @@ -990,7 +1002,7 @@

Everything else is good.  Thanks.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20111019/b3da8d41/attachment.sig>