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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9575
         if (isMaskRequired)
           NewSI = Builder.CreateMaskedStore(StoredVal, VecPtr, Alignment,
-                                            BlockInMaskParts[Part]);
----------------
can leave unchanged?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9718
+        // vectorization.
+        // FIXME: Better support reverse store after vp_reverse is added.
+        NewSI = lowerStoreUsingVectorIntrinsics(
----------------
Could you elaborate what better means here? Might have missed it, does the current code handle reverse?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9729
         NewSI = Builder.CreateMaskedScatter(StoredVal, VectorGep, Alignment,
-                                            MaskPart);
+                                            MaskValue(Part));
       } else {
----------------
can leave unchanged now?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9771
       Value *VectorGep = State.get(getAddr(), Part);
-      NewLI = Builder.CreateMaskedGather(DataTy, VectorGep, Alignment, MaskPart,
-                                         nullptr, "wide.masked.gather");
+      NewLI = Builder.CreateMaskedGather(DataTy, VectorGep, Alignment,
+                                         MaskValue(Part), nullptr,
----------------
can leave unchanged?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9780
         NewLI = Builder.CreateMaskedLoad(
-            DataTy, VecPtr, Alignment, BlockInMaskParts[Part],
+            DataTy, VecPtr, Alignment, MaskValue(Part),
             PoisonValue::get(DataTy), "wide.masked.load");
----------------
can leave unchanged?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:1089
+  VPValue *VPTrueMask = Plan.getVPValueOrAddLiveIn(TrueMask);
+  replaceHeaderPredicateWithIdiom(Plan, *VPTrueMask);
+  // Now create the ExplicitVectorLengthPhi recipe in the main loop.
----------------
fhahn wrote:
> Thinking a bit more about this, at the moment is only safe to replace the with TrueMask for VPWidenMemoryInstructionRecipes, for other recipes that use the mask it would be a potential mis-compile, correct? If not, then it would be good to have an assert that all users of the mask we replace are VPWidenMemoryInstructionRecipes.  Might be good to also have a test case for such a scenario 
Just do double-check as it looks like  the above may have been missed in the latest update, WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750