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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a reviewer: sanjoy.
sanjoy added a comment.
This revision now requires changes to proceed.

I have some comments / questions on the specification



================
Comment at: docs/LangRef.rst:12644
+Element Wise Atomic Memory Intrinsics
+-----------------------------
+
----------------
Add a couple of `-` s to underline the whole heading.


================
Comment at: docs/LangRef.rst:12646
+
+These intrinsics are similar to the standard library memory intrinsics. Only
+difference is that they perform memory transfer as a sequence of atomic
----------------
I'd change the style a little bit here --

"to the standard library memory intrinsics except that they perform"


================
Comment at: docs/LangRef.rst:12667
+                                              i1 <isvolatile>)
+      declare void @llvm.memcpy.element.atomic.p0i8.p0i8.i64(i8* <dest>, i8* <src>,
+                                              i64 <num_elements>, i32 <element_size>
----------------
Why not just have the `i64` variant?


================
Comment at: docs/LangRef.rst:12679
+element is accessed atomicly in source and destination buffers.
+
+Arguments:
----------------
s/atomicly/atomically/

Also, perhaps you meant "i.e." and not "e.g."?


================
Comment at: docs/LangRef.rst:12689
+
+``element_size`` should be a power of two, greater than zero and less than
+target-specific atomic access size limit.
----------------
*less than a target-specific atomic access size limit


================
Comment at: docs/LangRef.rst:12697
+
+If the ``isvolatile`` parameter is ``true``, this call is
+a :ref:`volatile operation <volatile>`.
----------------
Given that you're explicitly not passing this parameter to ``_llvm_memcpy_element_atomic``, perhaps this isn't needed?


================
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
----------------
"The same value may be read from the source buffer many times" >> why do you care about this?


================
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
----------------
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"


================
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.
+
----------------
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.


https://reviews.llvm.org/D27133