[PATCH] D26905: [SLP] Vectorize loads of consecutive memory accesses, accessed in non-consecutive (jumbled) way.

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

A few more cosmetic comments (sorry I didn't ask you to fix them all at once, but I keep noticing new ones every time I read the code).
Also, there's another thing I just realized is missing from this patch - we don't consider NeedToShuffle in getEntryCost().
You basically need to add the cost of a TTI::SK_PermuteSingleSrc shuffle to every NeedToShuffle load.

(Actually, it's a bit more complicated than that, since some of those shuffles may end up getting removed later, but it's probably better to be conservative here.)



================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1022
+                                            ScalarEvolution &SE) {
+  std::multimap<int, Value *> offValPair;
+  for (auto *Val : VL) {
----------------
mkuper wrote:
> Also, capitalization.
Also, this isn't a pair, it's a list of pairs.
OffValPairs can work.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2595
+        Value *Undef = UndefValue::get(VecTy);
+        Value *shuf = Builder.CreateShuffleVector((Value *)LI, Undef,
+                                                  ConstantVector::get(Mask));
----------------
shuf -> Shuf


================
Comment at: test/Transforms/SLPVectorizer/X86/jumbled-load.ll:51
   %mul.4 = mul i32 %load.1, %load.6
   %gep.7 = getelementptr inbounds i32, i32* %out, i64 0
   store i32 %mul.1, i32* %gep.7, align 4
----------------
ashahid wrote:
> ashahid wrote:
> > mkuper wrote:
> > > What happens if the stores are also out of order?
> > > (IIRC, we should already have code to deal with that, I just want to make sure it meshes with the stores being out of order correctly)
> > I have not checked yet, but I will check.
> It gels well with 'stores' being out-of-order by generating proper shufflemask for loads according to the out-of-order stores. 
I thought you added a test for the combination of out-of-order loads and out-of-order stores, but turns out I was imagining it. Could you please add one? 
(We should have a regression test making sure we don't generate extra shuffles.)


https://reviews.llvm.org/D26905