[PATCH] D29373: NVPTX: Extract mem intrinsic expansions into utilities

[Justin Lebar via Phabricator via llvm-commits]

jlebar added inline comments.


================
Comment at: include/llvm/Transforms/Utils/LowerMemIntrinsics.h:10
+//
+// Lower aggregate copies, memset, memcpy, memmov intrinsics into loops when the
+// size is large or is not a compile-time constant for targetsw without library
----------------
"Lower aggregate copies and memset, memcpy, and memmov intrinsics"?


================
Comment at: include/llvm/Transforms/Utils/LowerMemIntrinsics.h:10
+//
+// Lower aggregate copies, memset, memcpy, memmov intrinsics into loops when the
+// size is large or is not a compile-time constant for targetsw without library
----------------
jlebar wrote:
> "Lower aggregate copies and memset, memcpy, and memmov intrinsics"?
"lower to" rather than "lower into"?


================
Comment at: include/llvm/Transforms/Utils/LowerMemIntrinsics.h:10
+//
+// Lower aggregate copies, memset, memcpy, memmov intrinsics into loops when the
+// size is large or is not a compile-time constant for targetsw without library
----------------
jlebar wrote:
> jlebar wrote:
> > "Lower aggregate copies and memset, memcpy, and memmov intrinsics"?
> "lower to" rather than "lower into"?
We don't actually have explicit code here for lowering aggregate copies; maybe we should change that bit.


================
Comment at: include/llvm/Transforms/Utils/LowerMemIntrinsics.h:11
+// Lower aggregate copies, memset, memcpy, memmov intrinsics into loops when the
+// size is large or is not a compile-time constant for targetsw without library
+// support.
----------------
typo


================
Comment at: include/llvm/Transforms/Utils/LowerMemIntrinsics.h:27
+
+/// Expand instruction \p ConvertedInst into a loop implementing the semantics
+/// of llvm.memcpy with the equivalent arguments.
----------------
>From the impl we don't seem to "expand", "convert", or really do much of anything with ConvertedInst.  Should we call it "InsertBefore/After"?


================
Comment at: include/llvm/Transforms/Utils/LowerMemIntrinsics.h:29
+/// of llvm.memcpy with the equivalent arguments.
+void convertMemCpyToLoop(Instruction *ConvertedInst,
+                         Value *SrcAddr, Value *DstAddr, Value *CopyLen,
----------------
Seems like this one should be called createMemcpyLoop, createCopyLoop or something, while the other is called convertMemcpyToLoop.  I would probably also swap the collation order.


================
Comment at: include/llvm/Transforms/Utils/LowerMemIntrinsics.h:40
+                          unsigned SrcAlign, unsigned DestAlign,
+                          bool SrcIsVolatile, bool DstIsVolatile);
+void convertMemMoveToLoop(MemMoveInst *MemMove);
----------------
Do we really need these utilities for creating memmov and memset loops?  I guess they're kind of useful, but if nobody is using them our code isn't tested or anything, so maybe it's better to leave them out for now?


https://reviews.llvm.org/D29373