openshmem-org / specification

OpenSHMEM Application Programming Interface
http://www.openshmem.org
50 stars 32 forks source link

Clarify that in *mem routines, nelems is bytes #376

Closed jdinan closed 4 years ago

jdinan commented 4 years ago

User request to clarify that in the new *mem collective routines, nelems refers to number of bytes.

Ref: https://github.com/Sandia-OpenSHMEM/SOS/issues/936

jdinan commented 4 years ago

@nspark Can this be addressed by the collectives section committee?

nspark commented 4 years ago

Will do. It's worth noting that this ambiguity also exists for RMA routines.

nspark commented 4 years ago

@jdinan How does this look...?

broadcast-bytes

Or, would it be more clear to put this in the API description like this...?

broadcast-bytes-2

jdinan commented 4 years ago

What about in the apiargument description for nelems? I thought we had a routine that covers it there, but I'm not able to find it now.

    \apiargument{IN}{nelems}{The number of elements in \source and \dest arrays. For 
\FUNC{shmem\_broadcastmem}, elements are bytes and for an operation of fixed size $N$,
elements are $N/8$ bytes. }

This might be easy enough to apply to all of the *mem routines, what do you think?

nspark commented 4 years ago

I'm not crazy about those argument description sections, but it seems like a compact change.

jdinan commented 4 years ago

Thanks, agree that it's not ideal, but it looks like a change we can apply across the remaining routines without much trouble. If you'd like to include a top-level description, we could also add the nelems semantics to the front matter where we describe properties of buffers passed to OpenSHMEM operations.

jdinan commented 4 years ago

Pushing this issue to the RMA section, since the collective part of the fix is now resolved.

nspark commented 4 years ago

@shamisp Here's how we resolved this in the Collectives section: 30a0f0c35237f17fae9d96e4d34569b3f29f5092

shamisp commented 4 years ago

@jdinan do you have patch for this ?

jdinan commented 4 years ago

PR posted: https://github.com/shamisp/specification/pull/5