[loop-idiom] Support memcpy instructions for memmove-like loops
AbandonedPublic
Actions

Authored by zhuhan0 on Aug 19 2021, 12:24 AM.

Details

Reviewers

lebedev.ri
yurai007
xbolva00
• zino
wenlei
hoy
efriedma

Summary

Following https://reviews.llvm.org/D104464. In addition to load and store
instructions, also handle memcpy intrinsics in loop body. The constraints are
same as before (same base pointer, etc.).

Fixed the FIXME added in D104464, and added two more tests.

Diff Detail

Repository: rG LLVM Github Monorepo

Event Timeline

zhuhan0 created this revision.Aug 19 2021, 12:24 AM

Herald added subscribers: hoy, wenlei, hiraditya. · View Herald TranscriptAug 19 2021, 12:24 AM

zhuhan0 requested review of this revision.Aug 19 2021, 12:24 AM

Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2021, 12:24 AM

Herald added a subscriber: llvm-commits. · View Herald Transcript

zhuhan0 added reviewers: lebedev.ri, yurai007, xbolva00, • zino, wenlei, hoy.Aug 19 2021, 12:26 AM

zhuhan0 added a reviewer: efriedma.

Harbormaster completed remote builds in B120271: Diff 367410.Aug 19 2021, 12:56 AM

@zhuhan0: Thanks for following up my memmove change! Actually there is analogous follow up but it didn't catch lot of attention: https://reviews.llvm.org/D107075.
Maybe you can reuse some parts of that change. I feel you could at least copy most unit tests from there for better coverage.
And last but not least - there is one ongoing small but important fix for https://reviews.llvm.org/D104464 which can be found here: https://reviews.llvm.org/D107964
It would be great if you and @hoy could take a look since your changes depends on it.

yurai007 added inline comments.Aug 19 2021, 7:12 AM

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
1356	IIRC I think I had 'kind of' similar idea to use 4th mayLoopAccessLocation but since it seems expensive I ended up in 3 calls: https://reviews.llvm.org/D107075 I admit I didn't measure impact on compile time. Maybe it's not so bad.

In D108353#2954114, @yurai007 wrote:

@zhuhan0: Thanks for following up my memmove change! Actually there is analogous follow up but it didn't catch lot of attention: https://reviews.llvm.org/D107075.
Maybe you can reuse some parts of that change. I feel you could at least copy most unit tests from there for better coverage.
And last but not least - there is one ongoing small but important fix for https://reviews.llvm.org/D104464 which can be found here: https://reviews.llvm.org/D107964
It would be great if you and @hoy could take a look since your changes depends on it.

Oops I didn't know you had a follow-up patch already! Is it really okay for me to copy your diff? I can also review your diff and land that. Which do you prefer?

Oops I didn't know you had a follow-up patch already! Is it really okay for me to copy your diff? I can also review your diff and land that. Which do you prefer?

Well, it's up to you :) I don't have problem with abandoning https://reviews.llvm.org/D107075. But if you see there are benefits in (re)using it then go ahead.

https://reviews.llvm.org/D107075 landed.