This is an archive of the discontinued LLVM Phabricator instance.

NVPTX: Extract mem intrinsic expansions into utilities
ClosedPublic

Authored by arsenm on Jan 31 2017, 9:57 PM.

Details

Reviewers
jlebar

Diff Detail

Event Timeline

arsenm created this revision.Jan 31 2017, 9:57 PM
arsenm updated this revision to Diff 86580.Jan 31 2017, 10:46 PM

Add alignment arguments for future use

jlebar added inline comments.Feb 2 2017, 11:12 AM
include/llvm/Transforms/Utils/LowerMemIntrinsics.h
11

"Lower aggregate copies and memset, memcpy, and memmov intrinsics"?

11

"lower to" rather than "lower into"?

11

We don't actually have explicit code here for lowering aggregate copies; maybe we should change that bit.

12

typo

28

From the impl we don't seem to "expand", "convert", or really do much of anything with ConvertedInst. Should we call it "InsertBefore/After"?

30

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.

41

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?

41

If you wanted, you could keep these functions as file-private in the cc file. This way if we ever want to make them public, it's straightforward.

arsenm marked 5 inline comments as done.Feb 7 2017, 8:27 PM
arsenm added inline comments.
include/llvm/Transforms/Utils/LowerMemIntrinsics.h
41

I only left these because the NVPTX pass uses the memcpy function for the questionable handling of aggregate copies, and then kept it the same for the others. I can make them private.

arsenm updated this revision to Diff 87586.Feb 7 2017, 8:52 PM

Address comments

jlebar accepted this revision.Feb 8 2017, 9:06 AM

NVPTX pass uses the memcpy function for the questionable handling of aggregate copies

lol, but tell me what you really think. :)

include/llvm/Transforms/Utils/LowerMemIntrinsics.h
11

Reading it again, there are still some vestiges of where this file came from in this comment. Maybe

Utilities to convert memset, memcpy, and memmov to loops (e.g. for targets without library support) and to create a memcpy loop from whole cloth.

This revision is now accepted and ready to land.Feb 8 2017, 9:06 AM
arsenm closed this revision.Feb 8 2017, 10:02 AM
arsenm marked an inline comment as done.

r294490

include/llvm/Transforms/Utils/LowerMemIntrinsics.h
11

Changed except removed whatever "from whole cloth" was supposed to mean