[PATCH] D102834: [SLPVectorizer] Implement initial memory versioning.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/SLPVectorizer.h:59-60
+struct SLPVectorizerResult {
+  bool MadeAnyChange;
+  bool MadeCFGChange;
+
----------------
Convert this to enum? + add comments


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:692
 
+struct AccessInfo {
+  Value *UnderlyingObj;
----------------
Comments? Also, put it to a namespace


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:693-697
+  Value *UnderlyingObj;
+  const SCEV *PtrSCEV;
+  Type *AccessTy;
+
+  AccessInfo() : UnderlyingObj(nullptr), PtrSCEV(nullptr), AccessTy(nullptr) {}
----------------
Default initializers and defaulted constructor?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:706
+      if (auto *L = dyn_cast<LoadInst>(I)) {
+        if (!L->getType()->isVectorTy())
+          return {L->getPointerOperand(), L->getType()};
----------------
`isValidElementType`?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:710
+      if (auto *S = dyn_cast<StoreInst>(I))
+        if (!S->getValueOperand()->getType()->isVectorTy())
+          return {S->getPointerOperand(), S->getValueOperand()->getType()};
----------------
`isValidElementType`?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7657-7663
+    auto GetPtr = [](Instruction *I) -> Value * {
+      if (auto *L = dyn_cast<LoadInst>(I))
+        return L->getPointerOperand();
+      if (auto *S = dyn_cast<StoreInst>(I))
+        return S->getPointerOperand();
+      return nullptr;
+    };
----------------
Move out of the loop. Also, similar lambda is used in another place, worth creating a static function instead of lambdas.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7734
+  // checks.
+  SLPCost += 5 * PointerChecks.size() + MemBounds.size();
+  if (SLPCost >= ScalarCost) {
----------------
Do you consider the cost of all new check instructions as `1` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102834