[PATCH] D28909: [InstCombineCalls] Unfold element atomic memcpy instruction

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy accepted this revision.
sanjoy added a comment.

lgtm with nits inline



================
Comment at: include/llvm/IR/IntrinsicInst.h:171
+      assert(isa<ConstantInt>(Arg));
+      return cast<ConstantInt>(Arg)->getZExtValue();
+    }
----------------
No need to assert on `isa` -- that assert should implicitly happen during `cast<>`


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:66
+    cl::init(16),
+    cl::desc("Maximum number of elements in the atomic memcpy to allow "
+             "optimizer to unfold it into sequence of explicit loads and "
----------------
I'd rephrase this slightly as `"Maximum number of elements in atomic memcpies the optimizer is allowed to unfold"`.

(i.e. no need to get into details -- this is a hidden option that only people who know what they're doing should use)


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:118
 
+Instruction*
+InstCombiner::SimplifyElementAtomicMemCpy(ElementAtomicMemCpyInst *AMI) {
----------------
clang-format?


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:123
+  // First check that number of elements is compile time constant.
+  ConstantInt *NumElementsCI = dyn_cast<ConstantInt>(AMI->getNumElements());
+  if (!NumElementsCI)
----------------
Minor: use `auto *` here.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:133
+  // Don't unfold into illegal integers
+  uint64_t ElementSize = AMI->getElementSizeInBytes() * 8;
+  if (!AMI->getModule()->getDataLayout().isLegalInteger(ElementSize))
----------------
s/`ElementSize`/`ElementSizeInBits`/


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:134
+  uint64_t ElementSize = AMI->getElementSizeInBytes() * 8;
+  if (!AMI->getModule()->getDataLayout().isLegalInteger(ElementSize))
+    return nullptr;
----------------
There is a `DataLayout` in `InstCombiner`.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:166
+    // We know alignment of the first element. It is also guaranteed by the
+    // verifier that element size is less or equal than first element alignment
+    // and both of this values are powers of two.
----------------
I'd also prefer adding an assert here that checks what you said in the comment.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:170
+    // aligned.
+    Load->setAlignment(i == 0 ? AMI->getSrcAlignment()
+                              : AMI->getElementSizeInBytes());
----------------
Note: you can do better here -- e.g. if the source is aligned to 16 bytes, and the element size is 4 bytes, then instead the alignment being 16, 4, 4, 4, ... it can be 16, 4, 8, 4, 16, ... etc.

For now I'll just leave a TODO.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:182
+
+  // Set the number of elements of the copy to 0, it will be deleted on the
+  // next iteration.
----------------
Is it difficult to delete `AMI` right here?  If not, I'd just do that -- no point in waiting for another iteration if it is obvious what will happen.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1543
+  if (auto *AMI = dyn_cast<ElementAtomicMemCpyInst>(II)) {
+    if (Constant *C = dyn_cast<Constant>(AMI->getNumElements()))
+      if (C->isNullValue())
----------------
This is really a separate transform -- I'd prefer splitting it out and landing it independently.


https://reviews.llvm.org/D28909