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

Fri Oct 27 06:55:27 PDT 2023

ABataev added inline comments.

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9718
+        // vectorization.
+        // FIXME: Better support reverse store after vp_reverse is added.
+        NewSI = lowerStoreUsingVectorIntrinsics(
----------------
fhahn wrote:
> Could you elaborate what better means here? Might have missed it, does the current code handle reverse?
I disabled reverse support, see useVPWithVPEVLVectorization(), need support for vp_reverse intrinsic, which is not added yet.

================
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:
> 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?
Yes, missed it, will fix

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750