[m-rev.] For post-commit review: Loop control transformation, source-to-source component

[Paul Bone]

On Tue, Sep 27, 2011 at 04:21:42PM +1000, Zoltan Somogyi wrote:
> On 27-Sep-2011, Paul Bone <pbone at csse.unimelb.edu.au> wrote:
> > I'm committing this now so that Zoltan can begin to review it while I work on
> > the code generator component.
> 
> The first part of my post-commit review of Paul's loop control diff,
> covering everything except the transformation.
> 
> Paul, can you please attend to the new XXXs?

I'll address these in a future commit, probably after you finish your
post-commit review.

Thanks.

> 
> compiler/goal_util.m:
> 	Remove the new expand_plain_conj predicate Paul just added,
> 	since it exactly duplicates the existing goal_to_conj_list.
> 
> compiler/par_loop_control.m:
> 	Conform to the above.
> 
> runtime/mercury_par_builtin.h:
> 	Fix a bug introduced by Paul's diff: the extendable array MUST be
> 	the last slot in the MR_LoopControl structure.

    Thanks for spotting this.

> 
> 	Fix some of the documentation and the formatting.
> 
> runtime/mercury_par_builtin.c:
> 	Fix some of the documentation and the formatting.
> 
> 	Add some XXXs.
> 
> Index: runtime/mercury_par_builtin.c
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/runtime/mercury_par_builtin.c,v
> retrieving revision 1.4
> diff -u -b -r1.4 mercury_par_builtin.c
> --- runtime/mercury_par_builtin.c	27 Sep 2011 00:49:27 -0000	1.4
> +++ runtime/mercury_par_builtin.c	27 Sep 2011 06:13:29 -0000
> @@ -43,7 +43,7 @@
>  MR_LoopControl *
>  MR_lc_create(unsigned num_workers)
>  {
> -    MR_LoopControl* lc;
> +    MR_LoopControl  *lc;
>      unsigned        i;
>  
>      lc = MR_GC_malloc(sizeof(MR_LoopControl) +
> @@ -52,8 +52,10 @@
>      for (i = 0; i < num_workers; i++) {
>          /*
>          ** We allocate contexts as necessary, so that we never allocate a
> -        ** context we don't use.  Also by delaying the allocation of contexts
> +        ** context we don't use. Also, by delaying the allocation of contexts,
>          ** all but the first may execute in parallel with one-another.
> +        ** XXX I (zs) do not understand how the first half of the previous
> +        ** sentence implies the seconf half; I don't think it does.
>          */
>          lc->MR_lc_slots[i].MR_lcs_context = NULL;
>          lc->MR_lc_slots[i].MR_lcs_is_free = MR_TRUE;

It's not what I ment to say.  I've fixed it.

> @@ -78,7 +80,7 @@
>          unsigned i;
>  
>          /*
> -        ** Optimize this by using a hint to start the search at.
> +        ** XXX Optimize this by using a hint to start the search at.
>          */
>          for (i = 0; i<lc->MR_lc_num_slots; i++) {
>              if (lc->MR_lc_slots[i].MR_lcs_is_free) {

Done.

> Index: runtime/mercury_par_builtin.h
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/runtime/mercury_par_builtin.h,v
> retrieving revision 1.9
> diff -u -b -r1.9 mercury_par_builtin.h
> --- runtime/mercury_par_builtin.h	27 Sep 2011 00:49:27 -0000	1.9
> +++ runtime/mercury_par_builtin.h	27 Sep 2011 06:17:05 -0000
> @@ -431,7 +440,7 @@
>          }                                                                   \
>                                                                              \
>          /*                                                                  \
> -        ** Optimize this by using a hint to start the search at.            \
> +        ** XXX Optimize this by using a hint to start the search at.        \
>          */                                                                  \
>          for (i = 0; i<(lc)->MR_lc_num_slots; i++) {                         \
>              if ((lc)->MR_lc_slots[i].MR_lcs_is_free) {                      \

Done.

> @@ -446,6 +455,7 @@
>          ** Since only one context can ever run MR_lc_wait_free_slot then we \
>          ** can never fail to find a since outstanding workers can never be  \
>          ** incremented by another engine.                                   \
> +        ** XXX This comment is so mangled it does not make sense.           \
>          */                                                                  \
>          MR_fatal_error("No free slot found in loop control");               \
>      } while (0);

There was a break statement in the loop above, so the condition I was trying to
describe isn't true anyway.  Execution can lagitametly reach this point.  I've
removed the comment and the call to MR_fatal_error.

-------------- 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/20110928/2f6acd51/attachment.sig>