This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Mark memcpy/memmove/memset as thisreturn
ClosedPublic

Authored by jroelofs on Jul 2 2021, 1:15 PM.

Diff Detail

Unit TestsFailed

Event Timeline

jroelofs created this revision.Jul 2 2021, 1:15 PM
jroelofs requested review of this revision.Jul 2 2021, 1:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2021, 1:15 PM

SDAG doesn't seem to mark the call as returned with the same example. Is this a new optimization?

https://clang.godbolt.org/z/K49z51zoo

IIRC thisreturn comes up as a result of the returned attribute in IR, right? Should the frontend be marking memory functions as returned instead? (I guess that we'd need a patch like this either way...?)

arsenm added a subscriber: arsenm.Jul 2 2021, 1:42 PM
arsenm added inline comments.
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
597

Why change to unreachable? This is an unrelated change.

SDAG doesn't seem to mark the call as returned with the same example. Is this a new optimization?

https://clang.godbolt.org/z/K49z51zoo

IIRC thisreturn comes up as a result of the returned attribute in IR, right? Should the frontend be marking memory functions as returned instead? (I guess that we'd need a patch like this either way...?)

Godbolt is using c++ by default so we need to extern C the source: https://clang.godbolt.org/z/594M46vYs

But it seems the difference is that SDAG can tail call it while GISel doesn't.

SDAG doesn't seem to mark the call as returned with the same example.

It doesn't, but it must know something about this property since it doesn't restore $x0 afterward.

Is this a new optimization?

rdar://77466123

https://clang.godbolt.org/z/K49z51zoo

IIRC thisreturn comes up as a result of the returned attribute in IR, right? Should the frontend be marking memory functions as returned instead? (I guess that we'd need a patch like this either way...?)

Exactly. The thing that's special here is that these three functions have their own GISel Opcodes, which don't have that returned behavior that the call had, so it wasn't being transferred automatically like it would for other calls with the attribute.

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
597

The one call site only passes these four opcodes, so I figured this was a useful drive-by improvement. Happy to take it out if you think it should stay generic.

aemerson accepted this revision.Jul 20 2021, 4:10 PM
This revision is now accepted and ready to land.Jul 20 2021, 4:10 PM
This revision was landed with ongoing or failed builds.Jul 20 2021, 5:04 PM
This revision was automatically updated to reflect the committed changes.