Page MenuHomePhabricator

Correct lowering of memmove in NVPTX
ClosedPublic

Authored by eliben on Jul 15 2015, 7:59 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

eliben updated this revision to Diff 29782.Jul 15 2015, 7:59 AM
eliben retitled this revision from to Correct lowering of memmove in NVPTX.
eliben updated this object.
eliben added reviewers: jingyue, jholewinski.
eliben set the repository for this revision to rL LLVM.
eliben added a subscriber: llvm-commits.
jholewinski edited edge metadata.Jul 15 2015, 8:06 AM

The algorithm looks good. Thanks for working on this!

Though I'm wondering if this shouldn't be moved to the CodeGen library. Other targets may be able to benefit from this, like AMDGPU.

The algorithm looks good. Thanks for working on this!

Though I'm wondering if this shouldn't be moved to the CodeGen library. Other targets may be able to benefit from this, like AMDGPU.

Thanks for the quick review, Justin.

Re AMDGPU, I prefer not to generalize prematurely, because maybe YAGNI :-) AMDGPU folks may or may not need this... They are free to adopt and generalize this if they do need it and I'll be happy to help, of course.

jholewinski accepted this revision.Jul 15 2015, 8:31 AM
jholewinski edited edge metadata.

Fair enough.

This revision is now accepted and ready to land.Jul 15 2015, 8:31 AM

You may need this for non-clang frontends, but for clang, you can mark memmove as unsupported in TargetLibraryInfo. This is what we do on AMDGPU for memcpy and memset.

okwank added a subscriber: okwank.Jul 15 2015, 10:22 AM

Hi,

My name is Okwan Kwon, and I have two comments.

  1. Use getRawDest() and getRawSource() instead. getDest() and getSource() will strip off any casts to give the original pointer. When the original pointer type is not i8 *, then it will get an assertion.
  1. Can you find a way to make use of the alignment information from the memmove intrinsic? It might be possible to generate more efficient load/store than byte copying.

Okwan

jingyue added inline comments.Jul 15 2015, 10:22 AM
lib/Target/NVPTX/NVPTXLowerAggrCopies.cpp
125 ↗(On Diff #29782)

Argument names start with upper case.

168 ↗(On Diff #29782)

CreateInBoundsGEP(srcAddr, IndexPtr) should work now.

lib/Target/NVPTX/NVPTXTargetMachine.cpp
69 ↗(On Diff #29782)

Do you intend to use PR instead?

test/CodeGen/NVPTX/lower-aggr-copies.ll
21 ↗(On Diff #29782)

How come we don't see inbounds here? You created them using CreateInBoundsGEP.

eliben marked 4 inline comments as done.Jul 15 2015, 12:33 PM

Hi,

My name is Okwan Kwon, and I have two comments.

  1. Use getRawDest() and getRawSource() instead. getDest() and getSource() will strip off any casts to give the original pointer. When the original pointer type is not i8 *, then it will get an assertion.

Thanks, done. Also added test with casts.

  1. Can you find a way to make use of the alignment information from the memmove intrinsic? It might be possible to generate more efficient load/store than byte copying.

There's a TODO in the code now about this. I'll keep it as a TODO for now

lib/Target/NVPTX/NVPTXLowerAggrCopies.cpp
125 ↗(On Diff #29782)

True. For now I'm keeping consistent style with other functions in this file - which isn't great in terms of LLVM style. I will do a later refactoring to bring everything closer to the LLVM style

168 ↗(On Diff #29782)

Nice!

lib/Target/NVPTX/NVPTXTargetMachine.cpp
69 ↗(On Diff #29782)

Good catch, thanks.

test/CodeGen/NVPTX/lower-aggr-copies.ll
21 ↗(On Diff #29782)

So no, I didn't. This is still memcpy, not memmove. It didn't use inbounds. I will refactor it to use it in the future, because it makes sense here I think

eliben marked 4 inline comments as done.Jul 15 2015, 12:33 PM

Thanks for the comments

eliben updated this revision to Diff 29817.Jul 15 2015, 12:34 PM
eliben edited edge metadata.

Updated with comments

jingyue accepted this revision.Jul 15 2015, 9:16 PM
jingyue edited edge metadata.

LGTM

This revision was automatically updated to reflect the committed changes.