This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] AArch64LoadStoreOptimizer: fix bug in pre-inc check iterator
ClosedPublic

Authored by gberry on Feb 1 2016, 1:00 PM.

Details

Summary

Fix case where a pre-inc/dec load/store would not be formed if the
add/sub that forms the inc/dec part of the operation was the first
instruction in the block being examined.

Diff Detail

Event Timeline

gberry updated this revision to Diff 46572.Feb 1 2016, 1:00 PM
gberry retitled this revision from to [AArch64] AArch64LoadStoreOptimizer: fix bug in pre-inc check iterator.
gberry updated this object.
gberry added a subscriber: llvm-commits.
mcrosier added inline comments.Feb 1 2016, 1:09 PM
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
983

Did you mean to delete lines 954 and 955? Otherwise, I'm not sure why this check would be necessary.

1524

Space between the while and the (.

Or why not just while (MBBI != B)?

junbuml edited edge metadata.Feb 1 2016, 1:27 PM

Looks good to me assuming Chad's comment fixed.

gberry added inline comments.Feb 1 2016, 1:29 PM
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
983

No, lines 954, 955 filter out non-register operands, this new check filters out register operands of register 0, which is not a real register. These can show up in dbg_value instructions, which I removed as a special case in the loop below.
This idiom (check for reg operand, then check for reg != 0) appears in quite a few places.

1524

Yep and yep.

mcrosier accepted this revision.Feb 1 2016, 1:31 PM
mcrosier edited edge metadata.

LGTM once you've addressed the minor nits.

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
983

Ah, the subtle difference between !isReg and !getReg.. Thanks for the clarification.

This revision is now accepted and ready to land.Feb 1 2016, 1:31 PM
gberry updated this revision to Diff 46576.Feb 1 2016, 1:32 PM
gberry edited edge metadata.

Address Chad's comments.

This may need a trivial fix after r259828.

gberry updated this revision to Diff 46971.Feb 4 2016, 3:50 PM

Rebased after r259828

This revision was automatically updated to reflect the committed changes.