[PATCH] D99750: [LV, VP]VP intrinsics support for the Loop Vectorizer

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the updates! I think all correctness issues should now have been addressed AFAICT, some minor comments left inline



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9727
+        // vectorization.
+        // FIXME: Better support reverse store after vp_reverse is added.
+        NewSI = lowerStoreUsingVectorIntrinsics(
----------------
nit: Drop `better` here, as reverse storing isn't supported at all at the moment. Would be good to also assert.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9766
     Value *NewLI;
-    if (CreateGatherScatter) {
+      if (Value *EVLPart = State.EVL ? State.get(State.EVL, Part) : nullptr) {
+        // If EVL is not nullptr, then EVL must be a valid value set during plan
----------------
Indent looks off here


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9772
+        // vectorization.
+        // FIXME: Better support reverse loading after vp_reverse is added.
+        NewLI = lowerLoadUsingVectorIntrinsics(
----------------
nit: drop `Better` here, as reverse loading isn't supported at all a the moment. Would be good to assert here that the load isn't reversed


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:1020
+    if (OnlyWidenMemRecipes) {
+      for (unsigned J = 0; J < CompareToReplace->getNumUsers();) {
+        VPUser *User = CompareToReplace->user_begin()[J];
----------------
Would it be simpler just have a common function for finding all the header predicates and then have the code to update the users at the call sites?




================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:1023
+        unsigned NumUsers = CompareToReplace->getNumUsers();
+        if (!isa<VPWidenMemoryInstructionRecipe>(User)) {
+          ++J;
----------------
Added `VPValue::replaceUsesWithIf` in a002271972fb3fb2877bdb4abf9275b2c1291036 as there were already multiple places hand-rolling that functionality.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750