[PATCH] D27133: Introduce element-wise atomic memcpy and memmove intrinsics

[Igor Laevsky via Phabricator via llvm-commits]

igor-laevsky added inline comments.


================
Comment at: docs/LangRef.rst:12679
+element is accessed atomicly in source and destination buffers.
+
+Arguments:
----------------
sanjoy wrote:
> s/atomicly/atomically/
> 
> Also, perhaps you meant "i.e." and not "e.g."?
"E.g" is correct here. It is meant to describe one of the possible access patterns we can get. For example we can instead copy elements pair by pair if length is even. I replaced "e.g" with "for example" to make it more clear.


================
Comment at: docs/LangRef.rst:12710
+
+The order of the copy is unspecified. The same value may be read from the source
+buffer many times, but only one write is issued to the destination buffer per
----------------
reames wrote:
> sanjoy wrote:
> > sanjoy wrote:
> > > "The same value may be read from the source buffer many times" >> why do you care about this?
> > "The order of the copy is unspecified" -- I'd be a bit more explicit -- "The order in which the  ``num_elements`` elements are copied is unspecified"
> The ability to read multiple times let's you restart after inspecting the value.  Not sure why we'd want this, but let's leave the possibly available.  
> 
> (Maybe we can fastpath zero initialize if all the source values are zero or something?)
I have a feeling that new phrase implies that more (or less) elements might be copied. Why do you think it's important to rephrase current statement?


================
Comment at: docs/LangRef.rst:12716
+This intrinsic does not provide ordering with respect to other memory accesses
+in adjacent code. It can never be used to implement a fence-like construct.
+
----------------
reames wrote:
> sanjoy wrote:
> > I'd expect for this to at least participate with reordering constraints due to other memory operations.  That is, I'd expect reordering
> > 
> > ```
> > %val = acquire_load
> > atomic_memcpy(...)
> > ```
> > 
> > to
> > 
> > ```
> > atomic_memcpy(...)
> > %val = acquire_load
> > ```
> > 
> > to be illegal, even if the memory locations involved in memcpy is disjoint with the location in the load.
> Hm, good point.  This clearly needs reworded.  Possibly:
> This intrinsic does not provide any additional ordering guarantees over those provided by a set of unordered loads from the source location and stores to the destination.
Right. Idea here is that this intrinsic behaves like unordered memory operation. I've updated it with Philip's wording.


https://reviews.llvm.org/D27133