[PATCH] D33240: [Atomics] Rename and change prototype for atomic memcpy intrinsic

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Did a scan through the code, didn't spot anything major.  Once we settle the last few design/specification questions, this looks basically ready to go in.



================
Comment at: docs/LangRef.rst:14022
+                                                                       i32 <len>,
+                                                                       i1 <isvolatile>,
+                                                                       i8 <element_size>)
----------------
efriedma wrote:
> Hmm... I didn't mention isvolatile earlier?  See https://reviews.llvm.org/D27133?id=79305#629884 for original discussion of it.  At the very least, we need a better description of what it means.
I think we can just remove this.  The original motivation was essentially future proofing, and I don't think it's worth keeping the complexity for now.  We can change our minds later if it turns out we actually need this.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:109
+  uint8_t ElementSizeInBytes = AMI->getElementSizeInBytes();
+  uint64_t NumElements = LengthInBytes / ElementSizeInBytes;
   if (NumElements >= UnfoldElementAtomicMemcpyMaxElements)
----------------
skatkov wrote:
> assert LengthInBytes % ElementSizeInBytes == 0 and LengthInBytes > 0?
Agreed.  Also, length might actually be zero.  We should remove such calls.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1896
+  if (auto *AMI = dyn_cast<ElementUnorderedAtomicMemCpyInst>(II)) {
+    if (Constant *C = dyn_cast<Constant>(AMI->getLength()))
       if (C->isNullValue())
----------------
Hm, I might sink this into the helper function.  Optional, and can be submitted separately without further review.


https://reviews.llvm.org/D33240