This is an archive of the discontinued LLVM Phabricator instance.

ARMLoadStoreOptimizer: Create LDRD/STRD on thumb2
ClosedPublic

Authored by MatzeB on Jun 22 2015, 5:33 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 28180.Jun 22 2015, 5:33 PM
MatzeB retitled this revision from to ARMLoadStoreOptimizer: Create LDRD/STRD on thumb2.
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).

With a few nitpicks, looks good to me. John, any comments?

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
756 ↗(On Diff #28180)

nitpick: I'd have kept isi32Load(Opecode) in a bool IsLoad, and reused it on all cases.

945 ↗(On Diff #28180)

nitpick: variable name too close to CanMergeToLoadStoreMulti, confusing.

964 ↗(On Diff #28180)

this is a bit confusing... probably because of the close variable names, but also because of the chain of conditionals. Maybe it'll get better after you renamed the *This* variants to something smaller and more distinct.

1743 ↗(On Diff #28180)

better to leave if (! ...) here, not an empty block.

john.brawn added inline comments.Jun 24 2015, 10:15 AM
lib/Target/ARM/ARMLoadStoreOptimizer.cpp
935 ↗(On Diff #28180)

Only LDRD and not STRD, so should have || !isi32Load(Opcode)

945–961 ↗(On Diff #28180)

I think the logic here could be made much clearer by splitting it up something like

// Must access sequential ascending memory addresses
if (NewOffset != Offset + (int)Size)
  continue;
// Cannot load SP
if (Reg == ARM::SP)
  continue;
// VFP must load contiguous ascending registers, non-VFP must load ascending registers
CanMergeThisToLoadStoreMulti = (isNotVFP ? RegNum <= PRegNum : (Count < Limit) && RegNum == PRegNum+1))

The swift check I think can be done outside the loop, as it depends only on the first register. Instead of having CanMergeThisToLoadStoreDouble maybe just later set Candidate->CanMergeToLoadStoreDouble = CanMergeToLoadStoreDouble && Count == 2.

MatzeB updated this revision to Diff 28427.Jun 24 2015, 5:14 PM
MatzeB edited edge metadata.

Thanks for the reviews!

I put some of the cleanups suggested for FormCandidates() into the cleanup patch at http://reviews.llvm.org/D10140. The rest should be addressed in this new version.

rengolin added inline comments.Jun 25 2015, 5:48 AM
lib/Target/ARM/ARMLoadStoreOptimizer.cpp
761 ↗(On Diff #28427)

you could also use IsLoad here, too.

MatzeB updated this revision to Diff 28897.Jul 1 2015, 2:21 PM

Implement review suggestion.

rengolin accepted this revision.Jul 2 2015, 3:02 AM
rengolin edited edge metadata.

LGTM now. Thanks!

This revision is now accepted and ready to land.Jul 2 2015, 3:02 AM
MatzeB added a comment.Jul 2 2015, 2:57 PM

Thanks for the review. I can't commit this yet until http://reviews.llvm.org/D10140 is approved.

This revision was automatically updated to reflect the committed changes.