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

[Shahid via Phabricator via llvm-commits]

ashahid added inline comments.


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


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1019
+/// Returns the sorted memory accesses after sorting it.
+SmallVector<Value *, 8> llvm::SortMemAccess(ArrayRef<Value *> VL,
+                                            const DataLayout &DL,
----------------
mkuper wrote:
> The LLVM coding standard is that function names start with a non-capital, and variable names start with a capital.
> (There are some exceptions for functions, but this is mostly in old code.)
ok


================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1029
+    unsigned PtrBitWidth = DL.getPointerSizeInBits(AS);
+    Type *Ty = cast<PointerType>(Ptr->getType())->getElementType();
+    APInt Size(PtrBitWidth, DL.getTypeStoreSize(Ty));
----------------
RKSimon wrote:
> You are casting to PointerType and then only using it as a Type.
This is to resolve method membership error "class llvm::Type’ has no member named ‘getElementType" during compile time .


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:414
   /// Vectorize a single entry in the tree.
-  Value *vectorizeTree(TreeEntry *E);
+  Value *vectorizeTree(ArrayRef<Value *> VL, TreeEntry *E);
 
----------------
mkuper wrote:
> Please add an explanation for what the VL parameters means here.
Sure


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1257
       Type *SrcTy = VL0->getOperand(0)->getType();
       for (unsigned i = 0; i < VL.size(); ++i) {
         Type *Ty = cast<Instruction>(VL[i])->getOperand(0)->getType();
----------------
RKSimon wrote:
> for (unsigned i = 0, e = VL.size(); i < e; ++i) {
Do you want me to change the style of FOR statement to the above one?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2565
+      // must be followed by a 'shuffle' with the required jumbled mask.
+      if (E->NeedToShuffle && (VL.size() == E->Scalars.size())) {
+        SmallVector<Constant *, 8> Mask;
----------------
mssimpso wrote:
> I probably missed this, but why are we checking the sizes? Does this mean there will be cases where E->NeedToShuffle is true but we don't generate the shuffle?
No, I want to ensure that resulting vector type is not differing due to the length of the vector value.


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


https://reviews.llvm.org/D26905