Re: [Mpi3-rma] current version of proposal
Rajeev, Thank you very much for the comprehensive review! Comments are inline below:
5:16 - "void **base" should be "void *baseptr". (void * is used in Alloc_mem) fixed
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.
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
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
19:7 - "each buffer entry" -> "each entry in origin buffer" 19:12 - "each buffer entry" -> "each entry in result buffer" 19:19 - "each buffer entry" -> "each entry in target buffer" done
19:27 - delete * in "int *target_count" done
19:37 - "datatype of" should be "origin_datatype from" done
20:23 - "the buffer entry" -> "the entry in origin, result, and target buffers" done
20:42 - "datatype of" -> "datatype from" done
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
21:13 - "of buffer entry" -> "of the element in all buffers" done
21:33 - "target compare buffer" -> "target buffer" done
21:34 - change to "must belong to one of the following categories of predefined datatypes" done
22:42 - after "operation" add "(i.e., after the corresponding test or wait)" done
25:28 - delete * in "int *target_count" done
26:43 - Labeling problem: Figure 11.5 should be Figure 11.1 this seems correct in my draft?
26:12 - Delete the sentences from "This query functionality" onwards. There is no way to request a memory model, only to query it. So no similarity with threads. I tried to fix it but you're right, the next sentence is enough, so I removed it as suggested. I liked the analogy though.
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
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
37:14 - "refers to all" -> "refers to a lock on all" done
39:20 - awkward sentence. Change to "MPI_WIN_FLUSH completes all outstanding RMA operations initiated by the calling process to the target rank. The operations are completed both at the origin and at the target." Delete sentence "RMA operations..." done
40:1 - Sentence style is different from previous functions. Change to "Locally completes at the origin all..." done
46:9 - Delete the part "when the RMA operation completes at the target". This Rule 5 is about when a local update is visible in the public window. It has nothing to do with RMA operation completing at target. 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.
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).
47:42 - "based on avoiding" -> "to avoid" done
47:47 - "at the target" sounds like you are referring to another process, whereas you are referring to this process. Just delete it. done
48:1 - Remember to update Item 1 on previous page depending on the outcome of this. Right now it contradicts Item 1. yes, will be done later
49:18 - or unlock_all done
50:1, Example 11.12 - Lock_all and unlock_all are not collective. They are not needed on process B. Also, I would prefer the barrier to be before local_all on process A. Brian will fix the examples
50:2: Delete "Process B only needs to call lock_all..." Brian will fix the examples
51:1, Example 11.14: Same. Process B does not need to call lock_all, unlock_all. I would prefer the unlock_all on process A to be before the barrier. Brian will fix the examples
Typos/small fixes ------------- 1:39 - "in the following" -> "in this chapter" done 1:47 - add comma before "which" done 3:37 - delete "then" done 3:45 - add comma after "libraries". Delete "then" done 3:47 - add comma before "which" done 5:27 - "valid," -> "valid;" done 6:34 - "target. I.e," -> "target; i.e.," done 7:41 - why is "vendor" in green, should just be deleted done 7:43 - "MPI; that" -> "MPI, which" done 7:46 - "in" -> "on" done 7:47 - "that" -> "the" done 8:45 - "memory," -> "memory;" done 9:2 - "This, to" -> "This is to" done 9:5 - after "argument" add "to the corresponding window creation function" done 9:22 - delete first word "in" done 10:32 - "MPI wait" -> "MPI test or wait" done 19:34 - add space between last two variables done 21:2 - why is functionality in green. delete it. done 26:20 - "is" should be "are" done 26:21 - which -> that done 37:10 - delete green text done 38:4 - E.g. -> For example (a sentence shouldn't begin with E.g. or I.e.) done 38:25 - Delete "might". Should not be in green. done 39:2 - I prefer the MPI 2.2 version because "The update..." applies only to the last of the 3 options in the previous sentence. done 39:23 and 25 - delete green stuff; it wasn't in 2.2. done 39:24 - "document, this" -> "document, which" done 44:41 - Windows -> windows done 46:27 - add comma after "call" done 49:31 - "is" -> "are" I disagree, it doesn't have to delay them. 50:20 - "reading a" -> "reading of a" done 51:18 - proces -> process done 52:25 - interation -> interaction done 54:48 - add comma before which done
Thanks a lot Rajeev! I uploaded a new version to the Wiki. This version also contains parts of Jim's corrections. https://svn.mpi-forum.org/trac/mpi-forum-web/attachment/wiki/mpi3-rma-propos... 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
participants (1)
-
Torsten Hoefler