[Mpi3-rma] current version of proposal

[Torsten Hoefler]

On Fri, Mar 04, 2011 at 11:35:41PM -0600, Rajeev Thakur wrote:
> Thanks. A few more comments:
> 
> >> 5:28 - What discussion of alloc_mem in Section 8.2 also applies here? Is it the Rationale on the next page after free_mem? That should be mentioned here. It is important to mention here that void* actually means void**.
> > Yes, it's now referencing discussions and retionale.
> 
> I don't think the current sentence "The discussion of and rationales for MPI_ALLOC_MEM and MPI_FREE_MEM in Section 8.2 also apply to MPI_WIN_ALLOCATE" 
> 
> is enough for the user to realize there is important information back in Sec 8.2 that explains that void *baseptr is actually void **baseptr. And it's a common mistake users make when passing this parameter. How about adding this text "in particular... " at the end:
> 
> "The discussion of and rationales for MPI_ALLOC_MEM and MPI_FREE_MEM in Section 8.2 also apply to MPI_WIN_ALLOCATE; in particular, see the rationale in Section 8.2 for an explanation of the type used for \mpiarg{baseptr}. 
done

> >> 6:11 - Change "This is combined with local routines" to "It must be used in combination with local routines (MPI_Win_attach and MPI_Win_detach)"
> > done
> 
> Can you delete the parentheses around those functions (although they
> were there in my mail).
done - I also added a "the"

> >> 7:19 - Awkward sentence. Change to "The memory region specified must not contain any part that is already attached to the window win." Delete the part starting from "that is"
> > done
> 
> Change "are erroneous" to "is erroneous" in 7:21.
done

> >> 19:37 - "datatype of" should be "origin_datatype from"
> > done 
> 
> Bad line break now at 19:37.
Hmm, that's a macro thing :-(. Not sure what to do.

> >> 20:46 - awkward sentence. Change "and MPI_NO_OP or MPI_REPLACE" to "as well as MPI_NO_OP and MPI_REPLACE," (add comma at the end)
> > done
> 
> There should be a comma after MPI_REDUCE as well (as was there earlier).
done

> >> 29:43 - These two calls -> These four calls. Add sentence to cover lock_all and unlock_all
> > done
> > 
> >> 31:35 - comma after "needed". Delete then
> >> 31:36   - make it compare-and-swap
> > done
> 
> The last one is not done. I had suggested changing "compare and swap and accumulates" to "compare-and-swap and accumulates" (i.e., adding the hyphens) just to make the sentence read more clearly.
this is gone now 

> >> 37:11 - Awkward sentence. Change to "During the epoch, the calling process can access the window memory on all processes in win by using RMA operations."
> > done
> 
> win should be \mpiarg{win}
done

> >> 40:1 - Sentence style is different from previous functions. Change to "Locally completes at the origin all..."
> > done
> 
> Typo: origina -> origin
done 

> I would prefer "Locally completes" instead of "Locally complete". It is a short form of "This function locally completes..."
done 

> >> 47:14 - I would say delete this advice to implementors. If we keep it, I would like to fix the first sentence, which is awkward.
> > I fixed it.
> 
> "Overlapping accesses (and other operations that MPI-3 specifies) are undefined."
> It is not clear to the reader what "other operations" the text in parenthesis refers to.
right, I removed it.

> "However, implementations may wish to provide a mode in which such operations are erroneous to aid in debugging code."
> 
> "are erroneous to aid in debugging code" at the end of the sentence doesn't read well. I would change that sentence to 
> 
> "However, to assist users in debugging code, implementations may wish to provide a mode in which such operations are erroneous."
done

> >> 47:28 - why is "that use the same operation" being removed. It should be there.
> > no, this is now in the new info key. I added a sentence clarifying this
> > (and referencing back).
> 
> OK. With that deletion, the chapter doesn't say anywhere whether concurrent overlapping accumulates with different operators are allowed or not. Or are they allowed only if one of the two operations is a no_op?
> 
> And what is the default if the user doesn't pass any info key accumulate_ops? What should the implementation assume?
Right, I believe Brian is working on a fix (we should put this on the
list of open issues).

A new version is uploaded.

Thanks & All the Best,
  Torsten

-- 
 bash$ :(){ :|:&};: --------------------- http://www.unixer.de/ -----
Torsten Hoefler         | Performance Modeling and Simulation Lead
Blue Waters Directorate | University of Illinois (UIUC)
1205 W Clark Street     | Urbana, IL, 61801
NCSA Building           | +01 (217) 244-7736