This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Don't break alignment when combining base updates into load/stores.
ClosedPublic

Authored by ab on Dec 22 2014, 8:45 AM.

Details

Summary

r223862 caused a regression; quoting:
The selection logic for vld/vst doesn't care about less-than-standard alignment attributes. In this case, the alignment of the loads is 1, but SelectVLD selected VLD1q64, because the result type was v2i64. It was obvious from the testcases I added in that commit, but I didn't register the wrongness.

This patch introduces bitcasts if necessary, and changes the vld/vst type to one whose standard alignment matches the original load/store alignment.

Diff Detail

Repository
rL LLVM

Event Timeline

ab updated this revision to Diff 17562.Dec 22 2014, 8:45 AM
ab retitled this revision from to [ARM] Don't break alignment when combining base updates into load/stores..
ab updated this object.
ab edited the test plan for this revision. (Show Details)
ab added subscribers: Unknown Object (MLST), rs.
qcolombet accepted this revision.Dec 22 2014, 4:39 PM
qcolombet added a reviewer: qcolombet.
qcolombet added a subscriber: qcolombet.

Hi Ahmed,

LGTM with one small nitpick.

Thanks,
-Quentin

lib/Target/ARM/ARMISelLowering.cpp
8986 ↗(On Diff #17562)

Looking just at the possible code path, one may wonder why we do not need to handle V(ST|LD)xDUP.
Add a comment saying that by construction those are assumed to be properly aligned, that is why we just need to test for regular load/store.

This revision is now accepted and ready to land.Dec 22 2014, 4:39 PM
This revision was automatically updated to reflect the committed changes.