Page MenuHomePhabricator

[AArch64] Promote loads from stored
ClosedPublic

Authored by junbuml on Nov 21 2015, 7:31 AM.

Details

Summary

This change promotes load instructions which directly read from stored by
replacing them with mov instructions. If the store is wider than the load,
the load will be replaced with a bitfield extract.

For example :

STRWui %W1, %X0, 1
%W0 = LDRHHui %X0, 3

becomes

STRWui %W1, %X0, 1
%W0 = UBFMWri %W1, 16, 31

Diff Detail

Event Timeline

junbuml updated this revision to Diff 40869.Nov 21 2015, 7:31 AM
junbuml retitled this revision from to [AArch64] Promote loads from stored.
junbuml updated this object.
junbuml added a subscriber: llvm-commits.
mcrosier added inline comments.Nov 23 2015, 1:58 PM
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
1056

What if the store instruction modifies the base register (e.g., pre-/post-index)? I'm guessing the isMatchingStore makes that check unnecessary because it doesn't check for pre-/post-index versions of the store.

1496

Maximize 80-column.

junbuml updated this revision to Diff 40981.Nov 23 2015, 2:26 PM

Addressed Chad's comments

junbuml marked 2 inline comments as done.Nov 23 2015, 2:28 PM
junbuml added inline comments.
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
1056

Yes, you are right. pre-/post-index is not even considered, so BaseReg will not be modified by the store itself. Just in case, I added comment about it.

meadori added inline comments.
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
1550

This isn't completely accurate. In some cases the store is replaced with a move. I read this comment first and was surprised to find the move case in the actual code.

test/CodeGen/AArch64/arm64-ld-from-st.ll
604

I see lots of good tests to verify that the optimization happens, but I don't see any to verify cases where it should *not* happen (e.g. the volatile case, modified registers, in range, across calls, etc...). The negative cases are just as important to verify as the positive ones.

junbuml updated this revision to Diff 41418.Nov 30 2015, 11:56 AM
junbuml updated this object.
junbuml marked 3 inline comments as done.

Addressed Meador's comments.
Thanks Meador for your review!

Kindly ping ...

mcrosier accepted this revision.Dec 16 2015, 9:15 AM
mcrosier edited edge metadata.

LGTM, but please rename 'stored' to 'store' as suggested in the comments. I didn't enumerate all cases, so you might want to perform a mass replace.

I also assume you'd done extensive correctness testing. IIRC, you didn't see any major performance improvements/regressions.

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
46

s/stored/stores

98

Maximize 80-column.

102

s/Stored/Store

863

s/Stored/Store

This revision is now accepted and ready to land.Dec 16 2015, 9:15 AM
junbuml updated this revision to Diff 43063.Dec 16 2015, 2:31 PM
junbuml edited edge metadata.

Address Chad's comments.

I also assume you'd done extensive correctness testing. IIRC, you didn't see any major performance improvements/regressions.

I didn't see any correctness issue with this change in my correctness run for spec2000, spec2006, eembc, llvm-test. I didn't see significant performance changes.

I will commit this tomorrow if there are no further comments.

Thanks for the review!

junbuml closed this revision.Dec 18 2015, 10:14 AM

Landed in r256004. Thank you so much for the review !!