[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:9662
         Value *VectorGep = State.get(getAddr(), Part);
-        NewSI = Builder.CreateMaskedScatter(StoredVal, VectorGep, Alignment,
-                                            MaskPart);
+        if (Value *EVLPart = State.EVL ? State.get(State.EVL, Part) : nullptr) {
+          // If EVLPart is not null, we can vectorize using predicated
----------------
Great thanks! Now that there is VP intrinsic handling in multiple places, would it be better to handle all EVL related codegen together, i.e. something like below to avoid complicating reading the existing non-EVL code. WDYT?

```
for ()
   Value *VectorGep = State.get(getAddr(), Part);

   if (Value *EVLPart = State.EVL ? State.get(State.EVL, Part) : nullptr) {
      NewSI = lowerUsingVectorIntrinsics(vectorGEP..)
    } else {
      existing code....
    }
```



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9679
           // to reverse the order of elements in the stored value.
           StoredVal = Builder.CreateVectorReverse(StoredVal, "reverse");
           // We don't want to update the value in the map as it might be used in
----------------
Not sure if we also have a test case for this path, do you know if this would be handled correctly at the moment?


================
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.
----------------
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 


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:210
+  const VPlan *Plan = VPBB->getPlan();
+  if (Plan && Plan->getEntry()->getNumSuccessors() == 1) {
+    Header = Plan->getVectorLoopRegion()->getEntry();
----------------
ABataev wrote:
> fhahn wrote:
> > `Plan` here should never be `nullptr`
> It can be nullptr in the unit tests, had to add this to avoid crashes in unit tests.
Ah I see. Do you happen to know which unit tests? I suspect they need fixing and also checking why this isn't already caught by verification. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750