This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Use AEABI aligned function variants
ClosedPublic

Authored by john.brawn on Mar 4 2015, 8:00 AM.

Details

Summary

AEABI defines aligned variants of memcpy etc. that can be faster than the default version due to not having to do alignment checks. When emitting target code for these functions make use of these aligned variants if possible. Also convert memset to memclr if possible.

Diff Detail

Repository
rL LLVM

Event Timeline

john.brawn updated this revision to Diff 21203.Mar 4 2015, 8:00 AM
john.brawn retitled this revision from to [ARM] Use AEABI aligned function variants.
john.brawn updated this object.
john.brawn edited the test plan for this revision. (Show Details)
john.brawn set the repository for this revision to rL LLVM.
john.brawn added a subscriber: Unknown Object (MLST).

It may be worth noting that with glibc these functions may be no
better and in some cases slower than the standard ISO C functions.

Trunk glibc aliases aeabi_memcpy4 to aeabi_memcpy (and similarly
with the other functions) so it should be no slower as LLVM already
translates memcpy to __aeabi_memcpy for AEABI targets through the
generic Libcall lowering mechanism.

John

john.brawn updated this revision to Diff 21285.Mar 5 2015, 10:01 AM

New patch changes the decision of whether to do this to check what getLibcallName says the default function name is instead of duplicating the logic in ARMTargetLowering in case that logic changes.

john.brawn updated this revision to Diff 23727.Apr 14 2015, 5:28 AM

New patch that adjusts the tests to cope with changes since the last patch.

asl added a subscriber: asl.Apr 14 2015, 6:15 AM

Otherwise LGTM. Renato, what do you think?

lib/Target/ARM/ARMSelectionDAGInfo.cpp
36 ↗(On Diff #23727)

Minor nit: put brace on the preceding line

43 ↗(On Diff #23727)

Should we start with cheaper ABI check here and bail out early for non-AAPCS?

LGTM, Thanks!

john.brawn added inline comments.Apr 15 2015, 3:41 AM
lib/Target/ARM/ARMSelectionDAGInfo.cpp
36 ↗(On Diff #23727)

Will do.

43 ↗(On Diff #23727)

I'm not sure what you mean by this?

john.brawn updated this revision to Diff 24207.Apr 22 2015, 4:07 AM

Move curly brace position.

jroelofs added inline comments.
lib/Target/ARM/ARMSelectionDAGInfo.cpp
78 ↗(On Diff #24207)

No need for '{'s on single-statement if/else-if/else blocks.

104 ↗(On Diff #24207)

If it's already MVT::i32, don't generate the zext?

Remove unnecessary curly braces, only zext to i32 if < i32.

jroelofs accepted this revision.May 11 2015, 11:02 AM
jroelofs added a reviewer: jroelofs.
jroelofs added inline comments.
lib/Target/ARM/ARMSelectionDAGInfo.cpp
42 ↗(On Diff #25483)

I think @asl is suggesting:

if (!Subtarget.isAAPCS_ABI() ||
    std::strncmp(TLI->getLibcallName(LC), "__aeabi", 7) != 0)

here.

This revision is now accepted and ready to land.May 11 2015, 11:02 AM
john.brawn added inline comments.May 12 2015, 6:13 AM
lib/Target/ARM/ARMSelectionDAGInfo.cpp
42 ↗(On Diff #25483)

Ah, I see. Seems somewhat redundant to me (we would only get an aeabi function if the subtarget is already aapcs), and it's not like this is a time-critical function where avoiding a strncmp may be a good idea. I'll leave it as-is.

This revision was automatically updated to reflect the committed changes.