This is an archive of the discontinued LLVM Phabricator instance.

[LSV] Don't move stores across may-load instrs, and loosen restrictions on moving loads.
ClosedPublic

Authored by jlebar on Jul 19 2016, 4:06 PM.

Details

Summary

Previously we wouldn't move loads/stores across instructions that had
side-effects, where that was defined as may-write may-throw. But this
is both overly- and underly-restrictive:

  • Stores can't safely be moved across instructions that may load.
  • Loads can be moved across may-throw instructions; it's only may-write instructions that must be a barrier.

This patch also adds a DEBUG check that all instructions in our chain
are either loads or stores.

Diff Detail

Event Timeline

jlebar updated this revision to Diff 64589.Jul 19 2016, 4:06 PM
jlebar retitled this revision from to [LSV] Don't move stores across may-load instrs, and loosen restrictions on moving loads..
jlebar updated this object.
jlebar added a reviewer: asbirlea.
jlebar added subscribers: arsenm, llvm-commits.
jlebar updated this revision to Diff 64608.Jul 19 2016, 4:38 PM

Per dannyb's comment, don't move loads above throwing instructions. See rL270828 and PR 27858.

asbirlea added inline comments.Jul 20 2016, 11:04 AM
test/Transforms/LoadStoreVectorizer/NVPTX/merge-across-side-effects.ll
2

Could you double check the validation for these tests is done in getVectorizablePrefix, and there isn't an earlier exit on the data layout (just before the calls to getVectorizablePrefix)? Possibly add a data layout to avoid that, in case the defaults change?

jlebar added inline comments.Jul 20 2016, 11:22 AM
test/Transforms/LoadStoreVectorizer/NVPTX/merge-across-side-effects.ll
2

Could you double check the validation for these tests is done in getVectorizablePrefix

Sorry, not sure what you mean.

there isn't an earlier exit on the data layout

I think we should be good here because if the pass doesn't modify any of the functions, the test will also fail.

asbirlea added inline comments.Jul 20 2016, 11:28 AM
test/Transforms/LoadStoreVectorizer/NVPTX/merge-across-side-effects.ll
2

Ok, you're right, on early exit it wouldn't vectorize any of these.

asbirlea accepted this revision.Jul 20 2016, 11:29 AM
asbirlea edited edge metadata.
This revision is now accepted and ready to land.Jul 20 2016, 11:29 AM
This revision was automatically updated to reflect the committed changes.