This is an archive of the discontinued LLVM Phabricator instance.

[AArch64]Merge narrow zero stores to wider single store
ClosedPublic

Authored by junbuml on Nov 9 2015, 2:08 PM.

Details

Summary

This change merges adjacent zero stores into a wider store.
For example :

strh wzr, [x0]
strh wzr, [x0, #2]

becomes

str wzr, [x0]

This will fix PR25410.

Diff Detail

Event Timeline

junbuml updated this revision to Diff 39748.Nov 9 2015, 2:08 PM
junbuml retitled this revision from to [AArch64]Merge narrow zero stores to wider single store.
junbuml updated this object.
junbuml added reviewers: mcrosier, jmolloy, ab, mzolotukhin.
junbuml added subscribers: gberry, llvm-commits.
t.p.northover added inline comments.
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
721–722

What happens here for sequences like

strb wzr, [x0, #1]
strb wzr, [x0, #2]

? It looks like you might try to produce "strh wzr, [x0, #1]" (using STRHHui) which is invalid. This also applies to the code recently added around line 537.

junbuml added inline comments.Nov 10 2015, 8:03 AM
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
721–722

This pass will not merge the input you mentioned above. In findMatchingInsn(), we check if the new scaled wide load can express the scaled narrow inputs in around line 948 below in this patch.

t.p.northover added inline comments.Nov 10 2015, 9:53 AM
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
721–722

Ah, I see. Could we put some divisibility asserts in here just in case future modifications mess things around?

969

That hard-coded 2 looks very suspicious to me. Won't permit an incorrect STRHHui -> STRWui transformation?

junbuml added inline comments.Nov 10 2015, 10:46 AM
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
721–722

Sure. I will add assert here.

969

Since we merge two narrow zero stores to one zero store by doubling the datasize, I think 2 is just right value here for scaled case. For unscaled case, we don't need to have this check.

Since this patch transforms narrow stores which store only zero, I think STRHHui -> STRWui is also valid. Please let me know if there is any case I missed about the incorrect STRHHui -> STRWui transformation.

junbuml updated this object.Nov 10 2015, 10:48 AM
junbuml added a reviewer: t.p.northover.
junbuml removed a subscriber: t.p.northover.
t.p.northover added inline comments.Nov 10 2015, 11:42 AM
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
969

STRHHui -> STRWui is fine, but I think (can't test, the patch no longer applies cleanly to trunk) the following would go wrong:

strh wzr, [x0, #2]
strh wzr, [x0, #4]

In this case MinOffset == 2, alignTo(MinOffset, 2) == 2, but "str wzr [x0, #2]" isn't encodable.

junbuml added inline comments.Nov 10 2015, 11:59 AM
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
969

Sorry, I will rebase the patch soon.

In IR, above your scaled strh case will be looked like :

STRHHui %WZR, %X0, 1
STRHHui %WZR, %X0, 2

Therefore, MinOffset == 1 and alignTo(MinOffset, 2) != 2, so we will not allow this transformation.

t.p.northover accepted this revision.Nov 10 2015, 1:26 PM
t.p.northover edited edge metadata.

OK, I think this is good then. Thanks!

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
969

Oh, bother, yes. Thanks for explaining it to me. It might be worth renaming MinOffset to something that doesn't suggest it's the actual offset quite so strongly (MinOffsetImm, as above?).

I can do that after this patch has been committed though.

This revision is now accepted and ready to land.Nov 10 2015, 1:26 PM
junbuml updated this revision to Diff 40692.Nov 19 2015, 12:27 PM
junbuml edited edge metadata.

Rebase it before landing it.

Landed in r253711.
Thanks Tim for the review!

junbuml closed this revision.Nov 20 2015, 1:54 PM
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp