This is an archive of the discontinued LLVM Phabricator instance.

[MemLoc] Support lllvm.memcpy.inline in MemoryLocation::getForArgument
ClosedPublic

Authored by xbolva00 on Sep 19 2020, 10:32 AM.

Diff Detail

Event Timeline

xbolva00 created this revision.Sep 19 2020, 10:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2020, 10:32 AM
xbolva00 requested review of this revision.Sep 19 2020, 10:32 AM
fhahn added inline comments.Sep 20 2020, 3:38 AM
llvm/test/Transforms/DeadStoreElimination/MSSA/memintrinsics.ll
118

why pass -1 as length? That make the function be UB?

It would probably also be better to just pass in 2 noalias pointers, rather than allocas. That way the memcpy call won't be removed?

llvm/test/Transforms/DeadStoreElimination/MSSA/memset-and-memcpy.ll
103

nit: tab? Same below.

xbolva00 added inline comments.Sep 20 2020, 3:42 AM
llvm/test/Transforms/DeadStoreElimination/MSSA/memintrinsics.ll
118

-1 is also used at line 18.

fhahn added inline comments.Sep 20 2020, 3:51 AM
llvm/test/Transforms/DeadStoreElimination/MSSA/memintrinsics.ll
118

yeah, I don't think it really makes sense there either?

xbolva00 added inline comments.Sep 20 2020, 4:40 AM
llvm/test/Transforms/DeadStoreElimination/MSSA/memset-and-memcpy.ll
103

Original test case ("ret void" line) is wrongly formatted.

xbolva00 updated this revision to Diff 293015.Sep 20 2020, 4:50 AM

Addressed review comments.

xbolva00 marked 2 inline comments as done.Sep 20 2020, 4:51 AM
xbolva00 added inline comments.
llvm/test/Transforms/DeadStoreElimination/MSSA/memintrinsics.ll
22

hmm, no aliasing pointers - shouldn't we remove this store?

fhahn accepted this revision.Sep 20 2020, 4:56 AM

LGTM, thanks for also improving the test case!

llvm/test/Transforms/DeadStoreElimination/MSSA/memintrinsics.ll
22

Yes that looks like a missed optimization that was hidden by the fact that the store got removed because it was to an alloca that was never read! Thank you very much for improving the test so that it exposes this. Not sure what is going on, but it might be good to add a TODO/FIXME comment.

This revision is now accepted and ready to land.Sep 20 2020, 4:56 AM
This revision was automatically updated to reflect the committed changes.
xbolva00 marked an inline comment as done.

Thanks!

llvm/test/Transforms/DeadStoreElimination/MSSA/memintrinsics.ll
22

Added FIXME.