[PATCH] D56582: [InstCombine]Avoid introduction of unaligned mem access

Philip Reames via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 15 14:34:12 PST 2019


reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: test/Transforms/InstCombine/element-atomic-memintrins.ll:15
 
 define void @test_memset_to_store(i8* %dest) {
 ; CHECK-LABEL: @test_memset_to_store(
----------------
reames wrote:
> 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.
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.  


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56582/new/

https://reviews.llvm.org/D56582





More information about the llvm-commits mailing list