This is an archive of the discontinued LLVM Phabricator instance.

ARMLoadStoreOpt: Merge subs/adds into LDRD/STRD; Factor out common code
ClosedPublic

Authored by MatzeB on Jun 23 2015, 5:03 PM.

Details

Summary

This commit factors out common code from MergeBaseUpdateLoadStore() and
MergeBaseUpdateLSMultiple() and introduces a new function
MergeBaseUpdateLSDouble() which merges adds/subs preceding/following a
strd/ldrd instruction into an strd/ldrd instruction with writeback.

This requires http://reviews.llvm.org/D10623

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 28305.Jun 23 2015, 5:03 PM
MatzeB retitled this revision from to ARMLoadStoreOpt: Merge subs/adds into LDRD/STRD; Factor out common code.
MatzeB updated this object.
MatzeB edited the test plan for this revision. (Show Details)
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: Unknown Object (MLST).
rengolin edited edge metadata.Jul 2 2015, 3:53 AM

Hi Matthias,

Nice work, thanks! Apart from a few nitpicks, this looks good to me.

cheers,
--renato

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
1123

Isn't there a way to merge these two functions in one?

1772

What if the patterns you're merging up on line 1756 are also in this list? Shouldn't MergeBaseUpdateLSDouble() remove the match from the list and then you won't need to do this? Or is this what the test on "Imm != 0 -> return" on line 1365 is doing?

MatzeB added a comment.Jul 2 2015, 3:34 PM

Thanks for the review, comments below:

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
1123

Actually in some intermediate versions I had merged the two. The problem is however that after finding a incdec before you have to decide on different factors whether you can actually fold that increment/decrement in and if you cannot you go on searching one after.
These factors however vary between each of the three users, so to merge the two functions you need to pass in some predicate lambdas or bitsets of which inc/decs are allowed. That turned out to be messier than simply having two functions.

1772

I'm not sure I understand your concerns. In anyway this second loop and the MergeBaseCandidates list requires a comment (which I will add): Currently there is a LDRD/STRD formation pass that runs before register allocation. Unfortunately I cannot disable that one yet, as it is less dependent on the schedule and manages to catch some cases which the late peephole can't when the loads/stores aren't scheduled next to each other.
Anyway the previously formed LDRD/STRD instructions will end up in this list, the ones that are formed by the MergeOpsUpdate() are handled on line 1750.

In the long term I would like to disable the pre-RA formation of LDRD/STRD but that would require to enhance the ARMLoadStoreOpt pass to reschedule instruction. I've actually worked on that but could not get it into a state yet where it doesn't occasionally perform an invalid transformation.

rengolin accepted this revision.Jul 3 2015, 4:58 AM
rengolin edited edge metadata.

LGTM, with the comment. Thanks!

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
1123

I thought as much... Since they're close to each other, there will be less chance for changing one and not the other.

1772

Ok, a comment to that effect would be nice.

This revision is now accepted and ready to land.Jul 3 2015, 4:58 AM
This revision was automatically updated to reflect the committed changes.