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

[Daniel Neilson via Phabricator via llvm-commits]

dneilson added inline comments.


================
Comment at: docs/LangRef.rst:14022
+                                                                       i32 <len>,
+                                                                       i1 <isvolatile>,
+                                                                       i8 <element_size>)
----------------
reames wrote:
> 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.
Agreed. I'll remove it.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:4878
 
-    Entry.Ty = I.getArgOperand(2)->getType();
-    Entry.Node = NumElements;
+    Entry.Ty = MI.getLength()->getType();
+    Entry.Node = Length;
----------------
skatkov wrote:
> MI.getLength() == Length
Not quite. Doesn't seem to be a straightforward way to go from an SDValue to a Type*, so I don't think this sort of replacement can be made.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:109
+  uint8_t ElementSizeInBytes = AMI->getElementSizeInBytes();
+  uint64_t NumElements = LengthInBytes / ElementSizeInBytes;
   if (NumElements >= UnfoldElementAtomicMemcpyMaxElements)
----------------
reames wrote:
> skatkov wrote:
> > assert LengthInBytes % ElementSizeInBytes == 0 and LengthInBytes > 0?
> Agreed.  Also, length might actually be zero.  We should remove such calls.
Re: The assert. I think that it would be better to check this in the verifier. In the LangRef we've said that it's undefined behaviour if length isn't a multiple of element size, so I think it's okay to blindly do this divide here and add a check to the verifier.

Re: Zero length; see the in-line comment below.


================
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())
----------------
reames wrote:
> Hm, I might sink this into the helper function.  Optional, and can be submitted separately without further review.
This is following the same pattern/code-flow as the normal memcpy/memmove/memset handlers just above this. i.e. Check for a null length -- if there is one, then remove the call, else call the simplify method for the intrinsic. I'm inclined to stick to this pattern to make the later merging of the introspection classes cleaner.


https://reviews.llvm.org/D33240