Page MenuHomePhabricator

[AArch64] Promote loads from stored

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



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


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

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.


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.

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.

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.


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.




Maximize 80-column.





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 !!