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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

This looks generally good to me, but it would be good to get a blessing from @ABataev too.

Ideally I would like to see two things changed before this is committed:

- Start with this enabled by default, like I also mentioned in previous comments,
- I added a comment about the style of `vectorizeBlockWithVersioning`, which is too big for my liking, see also the comment inlined. I think that could do with a refactoring, and it was almost a blocker for me, but not entirely. :-)



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7957
+
+SLPVectorizerResult SLPVectorizerPass::vectorizeBlockWithVersioning(
+    BasicBlock *BB, const SmallPtrSetImpl<Value *> &TrackedObjects,
----------------
Style: this function is nearly 300 lines, which I think is big for a function, and it is already not the easiest function to read. I would prefer to see this split up and using helper functions.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:8294
+      if (EnableMemoryVersioning)
+        R.CollectMemAccess = BB->size() <= 300;
+
----------------
Make this an internal option, or a `static constexpr`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102834