This is an archive of the discontinued LLVM Phabricator instance.

[LoadStoreVectorizer] Enable vectorization of stores in the presence of an aliasing load
ClosedPublic

Authored by asbirlea on Nov 22 2016, 3:32 PM.

Details

Summary

The "getVectorizablePrefix" method would give up if it found an aliasing load for a store chain.
In practice, the aliasing load can be treated as a memory barrier and all stores that precede it
are a valid vectorizable prefix.
Issue found by volkan in D26962. Testcase is a pruned version of the one in the original patch.

Event Timeline

asbirlea updated this revision to Diff 78967.Nov 22 2016, 3:32 PM
asbirlea retitled this revision from to [LoadStoreVectorizer] Enable vectorization of stores in the presence of an aliasing load.
asbirlea updated this object.
asbirlea added subscribers: llvm-commits, volkan, anna.
jlebar added inline comments.Nov 22 2016, 3:58 PM
lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
486

Nit, pls end sentences with periods.

490

Can we add a comment on this loop to the effect of:

Find the first instruction (in BB order) which "conflicts with" any of ChainInstrs[0..ChainInstrIdx].

529

As discussed IRL, not sure why we need this early-break at all.

asbirlea added inline comments.Nov 22 2016, 4:32 PM
lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
529

This a legality condition. It relates to the above comment:
"We can ignore the alias as long as the load comes before the store, because that means we won't be moving the load past the store [...]"

We consider loads before stores safe. This patch will somewhat relax this condition, but only for stores. First, store chains:
If a load comes before the store it's safe per above comment/condition. If a load comes after the store, we allow some leniency and don't break right away to enable vectorization of all stores that precede the "guilty" load.
Now, for load chains:
If the load comes before, again it's safe. But if we find a store that aliases, we _must_ stop processing and not take any loads that follow that store. The vectorized load will be moved at the location of the first load found, pulling up a load past an aliasing store. This produces incorrect code if the load it pulls up aliases the store(s) above it, a check that we don't make right now.
There may be room for further improvement with additional checks, but for now, don't do this.

asbirlea updated this revision to Diff 79003.Nov 22 2016, 5:11 PM
asbirlea edited edge metadata.

Add more comments.

asbirlea marked 4 inline comments as done.Nov 22 2016, 5:13 PM
jlebar accepted this revision.Nov 22 2016, 5:41 PM
jlebar edited edge metadata.

As we discussed, I want to try splitting out this loop into a load and a store version to see if it clarifies things, but I'm happy to do that as a separate patch. This lgtm.

lib/Transforms/Vectorize/LoadStoreVectorizer.cpp
533

I'd either get rid of the second paragraph here or encode it as an assert.

This revision is now accepted and ready to land.Nov 22 2016, 5:41 PM
asbirlea updated this revision to Diff 79114.Nov 23 2016, 9:50 AM
asbirlea edited edge metadata.

Added assert and comment to justify it.

asbirlea updated this revision to Diff 79115.Nov 23 2016, 9:51 AM
asbirlea edited edge metadata.

Period.

This revision was automatically updated to reflect the committed changes.