[PATCH] D90445: [SLP] Make SLPVectorizer to use `llvm.masked.gather` intrinsic

[Anton Afanasyev via Phabricator via llvm-commits]

anton-afanasyev added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:1580
 
+    /// Boolean value indicating that pointer operands are scattered
+    bool IsScatteredOps = false;
----------------
RKSimon wrote:
> scattered.
Ok


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:2848
+        // Vectorizing non-consecutive loads with `llvm.masked.gather`
+        TreeEntry *TE = newTreeEntry(VL, Bundle /*vectorized*/, S, UserTreeIdx,
+                                     ReuseShuffleIndicies);
----------------
RKSimon wrote:
> 'llvm.masked.gather'.
Ok


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4517
+          if (getTreeEntry(PO))
+            ExternalUses.push_back(ExternalUser(PO, cast<User>(VecPtr), 0));
+        }
----------------
RKSimon wrote:
> Should this be 0? Isn't it supposed to be the LaneIndex?
Sure, thanks!


================
Comment at: llvm/test/Transforms/SLPVectorizer/X86/pr47629.ll:17
+; CHECK-NEXT:    store <4 x i32> [[TMP11]], <4 x i32>* [[TMP12]], align 4
 ; CHECK-NEXT:    ret void
 ;
----------------
RKSimon wrote:
> This doesn't look great in the final codegen: https://gcc.godbolt.org/z/vE9Yoe
> 
> Which suggests either the costs aren't correct or we're not correctly including the cost of something - the buildvector of the pointers? are we missing getelementptr vectorization?
Oops, thanks, it looks I've missed the buildvector cost.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90445/new/

https://reviews.llvm.org/D90445