This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine]Avoid introduction of unaligned mem access
ClosedPublic

Authored by skatkov on Jan 11 2019, 1:03 AM.

Details

Summary

InstCombine is able to transform mem transfer instrinsic to alone store or store/load pair.
It might result in generation of unaligned atomic load/store which later in backend
will be transformed to libcall. It is not an evident gain and it is better to keep intrinsic as is
and handle it at backend.

Diff Detail

Repository
rL LLVM

Event Timeline

skatkov created this revision.Jan 11 2019, 1:03 AM
reames requested changes to this revision.Jan 11 2019, 12:33 PM

Code looks reasonable, but please see comment on testing strategy.

test/Transforms/InstCombine/element-atomic-memintrins.ll
15 ↗(On Diff #181218)

I think these changes break the spirit of the tests. I'd recommend just providing the required alignment information on the dest argument. And then add a separate test for the unaligned case.

This revision now requires changes to proceed.Jan 11 2019, 12:33 PM
skatkov updated this revision to Diff 181494.Jan 13 2019, 6:15 PM

Philip, I'm not sure that I completely follow your comment about spirit of the tests but I've added a coverage for size/alignment.
I hope it will be enough.

reames accepted this revision.Jan 15 2019, 2:34 PM

LGTM

test/Transforms/InstCombine/element-atomic-memintrins.ll
15 ↗(On Diff #181218)

I was suggesting that you add an align(8) to the %dest argument. This would preserve the transform for the existing tests, and you could add a new test showing the inhibited transform w/o the alignment. Reviewing your tests, I think you do have the coverage, just not quite the way I was suggesting. So, consider this an optional comment.

This revision is now accepted and ready to land.Jan 15 2019, 2:34 PM
This revision was automatically updated to reflect the committed changes.