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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

I have now read the code for the first time, and here's a first round of comments.
I definitely need to read this again, which is what I will do soon...



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:189
+static cl::opt<bool> EnableMemoryVersioning(
+    "slp-memory-versioning", cl::init(false), cl::Hidden,
+    cl::desc("Enable memory versioning for SLP vectorization."));
----------------
This is a nitpick, bikeshedding names... but I was wondering about the terminology here (i.e. memory versioning).
We emit runtime memory checks, and do versioning of a block. So was wondering if this should be BlockVersioning. 


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6999
+              if (!DestBundle->IsScheduled && SLP->CollectMemAccess) {
+                // FIXME Naming
+                auto GetPtr = [](Instruction *I) -> Value * {
----------------
Remove this?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7611
+    Tail = SplitBlock(BB, LastTrackedInst->getNextNode(), &DTU, LI, nullptr,
+                      OriginalBBName + ".tail");
+  auto *CheckBlock = BB;
----------------
Don't think I have seen a block name with the .tail suffix in the tests.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7647
+
+  // Add !noalias metadata to memory accesses in the versiond block.
+  LLVMContext &Ctx = BB->getContext();
----------------
typo: versiond -> versioned.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7650
+  MDBuilder MDB(Ctx);
+  MDNode *Domain = MDB.createAnonymousAliasScopeDomain("SLPVerDomain");
+
----------------
I am not that familiar with this, but do we expect this in one of the tests?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7735
+  SLPCost += 5 * PointerChecks.size() + MemBounds.size();
+  if (SLPCost >= ScalarCost) {
+    // Vectorization not beneficial or possible. Restore original state by
----------------
Perhaps create a local helper function to handle this case.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7736
+  if (SLPCost >= ScalarCost) {
+    // Vectorization not beneficial or possible. Restore original state by
+    // removing the introduced blocks.
----------------
Re: "or possible", I would guess it's possible but not profitable?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7763
+    NumVersioningFailed++;
+  } else {
+    R.getORE()->emit(
----------------
And another helper for this case too.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6843
 
-bool SLPVectorizerPass::runImpl(Function &F, ScalarEvolution *SE_,
-                                TargetTransformInfo *TTI_,
-                                TargetLibraryInfo *TLI_, AAResults *AA_,
-                                LoopInfo *LI_, DominatorTree *DT_,
-                                AssumptionCache *AC_, DemandedBits *DB_,
-                                OptimizationRemarkEmitter *ORE_) {
+SLPVectorizerResult SLPVectorizerPass::vectorizeBlockWithVersioning(
+    BasicBlock *BB, const SmallPtrSetImpl<Value *> &TrackedObjects,
----------------
Somewhere a high-level idea and description of the algorithm would be nice, I guess that would be here.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6850
+
+  // First, clean up delete instructions, so they are not re-used during SCEV
+  // expansion.
----------------
Typo? delete -> deleted.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6870
+
+    unsigned AS = Obj->getType()->getPointerAddressSpace();
+
----------------
Unused?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6872
+
+    // We know that the Start is dereferenced, hence adding one should not
+    // overflow: https://alive2.llvm.org/ce/z/dTuGLx
----------------
That wasn't obvious to me, why is Start dereferenced?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6873
+    // We know that the Start is dereferenced, hence adding one should not
+    // overflow: https://alive2.llvm.org/ce/z/dTuGLx
+    auto &DL = BB->getModule()->getDataLayout();
----------------
Do we want to link to alive (not sure how persistent the data is and if it's common), or just expand the example here?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6900
+    return all_of(make_range(FirstTrackedInst->getIterator(),
+                             std::next(LastTrackedInst->getIterator())),
+                  [LastTrackedInst, BB](Instruction &I) {
----------------
Could `std::next` return null (or end)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102834