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

[Shahid via Phabricator via llvm-commits]

ashahid added a comment.

ping!



================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1022
+                                        ScalarEvolution &SE) {
+  std::multimap<int, Value *> offValPair;
+  for (unsigned i = 0; i < VL.size(); i++) {
----------------
mkuper wrote:
> ashahid wrote:
> > mkuper wrote:
> > > ashahid wrote:
> > > > mkuper wrote:
> > > > > Why is this a multimap?
> > > > This is because the elements in the multimap follow a certain order, so using this will ensure that the values are sorted accordingly.
> > > It doesn't seem right to use a multimap just for the sorting behavior.
> > > I think you can find a more appropriate container. See http://llvm.org/docs/ProgrammersManual.html#picking-the-right-data-structure-for-a-task
> > I did refer to this manual but I could not find some thing similar.I am curious, what issue do you see with the usage of multimap? BTW, If you have any specific container in your mind, pls let me know.
> Well, first, I don't believe you actually need a *mutli*map here, right?
> We don't actually expect to get several elements with the same offset,  we can fail immediately if that happens. So, you could replace the multimap with a regular map, and a check for the "multi" condition.
> 
> Assuming I'm not missing anything about that, the options are basically either a regular std::map, or a sorted vector ( http://llvm.org/docs/ProgrammersManual.html#dss-sortedvectormap )
Agreed. Updating the patch accordingly.


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1033
+    Ptr = Ptr->stripAndAccumulateInBoundsConstantOffsets(DL, Offset);
+    const SCEV *OffsetSCEV = SE.getConstant(Offset);
+    auto *OffsetC = dyn_cast<SCEVConstant>(OffsetSCEV);
----------------
mkuper wrote:
> Why are you turning a constant into a SCEV and back into a constant?
My bad, refactored accordingly.


================
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:
> 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. 


https://reviews.llvm.org/D26905