This fixes https://llvm.org/bugs/show_bug.cgi?id=24056
Also a bit of refactoring along the way.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
Hi,
My name is Okwan Kwon, and I have two comments.
- 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.
- 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
| 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. |
Comment Actions
Thanks, done. Also added test with casts.
- 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 |