This is an archive of the discontinued LLVM Phabricator instance.

[AARCH64] Gang up loads and stores (for memcpy) for pairing.
ClosedPublic

Authored by SirishP on May 4 2018, 2:20 PM.

Details

Summary

This is continuation of https://reviews.llvm.org/D45098. This patch gangs up loads and stores for pairing (in case of memcpy). We address only memcpy and not other memory operations, because in mempy source and destination addresses are disjoint and hence no alias analysis is required to disambiguate loads and stores.

Each target defines numbers of loads and stores to be ganged up. For Aarch64, it is set at 4. Other targets have defaulted to 0, and hence no effect.

Diff Detail

Event Timeline

SirishP created this revision.May 4 2018, 2:20 PM
sebpop added inline comments.May 8 2018, 9:24 AM
llvm/include/llvm/CodeGen/TargetLowering.h
2524

s/inline/inlined/

2525

s/that you can want//
s/tother/together/

2526

s/vectorazation/vectorization/

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
92

To avoid double negative, which is confusing, please rename this command line option as EnableMem... .

95

remove this second part of the sentence.

5361

Remove empty line.

5394

remove parentheses

5399

The first part is not a sentence, please rewrite in a non-ambiguous way.
s/maybe/may be/
s/mempy/memcpy/

5403

clang-format

5409

clang-format

5421

remove parentheses

5422

remove parentheses

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
586

Let's commit this change https://reviews.llvm.org/D45098 separately after this patch lands.

llvm/test/CodeGen/AArch64/arm64-memset-to-bzero.ll
11 ↗(On Diff #145259)

You won't need to change this testcase if you remove the change to "MaxStoresPerMemset = 32;"

89 ↗(On Diff #145259)

Do we need to change the testcase here? Why?

llvm/test/CodeGen/AArch64/arm64-misaligned-memcpy-inline.ll
10 ↗(On Diff #145259)

Do we need to change the testcase here? Why?

SirishP marked 13 inline comments as done.May 14 2018, 1:24 PM
SirishP added inline comments.
llvm/test/CodeGen/AArch64/arm64-memset-to-bzero.ll
89 ↗(On Diff #145259)

This was done due to change in maxstore per memset. If we change it back to the original value, we don't need this change for now.

SirishP retitled this revision from [AARCH64] Change max stores for memcpy/memmov/memset and gang up loads and stores (for memcpy) for pairing. to [AARCH64] Gang up loads and stores (for memcpy) for pairing..May 14 2018, 1:26 PM
SirishP edited the summary of this revision. (Show Details)
SirishP marked 2 inline comments as done.
SirishP updated this revision to Diff 146672.May 14 2018, 1:37 PM
sebpop accepted this revision.May 14 2018, 2:17 PM

LGTM with some minor changes.

llvm/include/llvm/CodeGen/TargetLowering.h
2525

s/that you can want//

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
5241

Dot at the end of a sentence: "Use LoadToken to chain all loads."

5399

s/mempy/memcpy/

5409

else should be on the same line as close brace:

} else {
5429

Put a dot at end of a sentence.

This revision is now accepted and ready to land.May 14 2018, 2:17 PM
SirishP updated this revision to Diff 146689.May 14 2018, 2:31 PM
SirishP marked 5 inline comments as done.

You know you can either just use arc patch, and automagically get a
nice commit msg,
or at least manually add "Differential Revision: link", and it will
get closed automatically?

You know you can either just use arc patch, and automagically get a
nice commit msg,
or at least manually add "Differential Revision: link", and it will
get closed automatically?

yup, the problem is when people commit with a message containing the magic keywords and forget a column...