[PATCH] D99750: [LV, VP] RFC: VP intrinsics support for the Loop Vectorizer (Proof-of-Concept)

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D99750#4352773 <https://reviews.llvm.org/D99750#4352773>, @ABataev wrote:

> In D99750#4350780 <https://reviews.llvm.org/D99750#4350780>, @fhahn wrote:
>
>> The patch seems to add code for using VP for both masking and EVL, but the masking part seems to be mostly unused in the current version. Would it be possible to disentangle them?
>
> I rather doubt it is possible, otherwise the code won't work correctly, at all. Masking is important part of predicated intrinsics.

Would it work to unconditionally set `    Builder.setMask(AllTrue);`? IIUC it should be controlled by EVL



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2886
+    Value *TC = getTripCount();
+    // Loop has multiple exits. Make sure scalar remainder executes at least 1
+    // scalar iteration to perform correct jump.
----------------
Is there a test with multiple exits?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5224
+    if (UserIC > 1) {
+      LLVM_DEBUG(dbgs() << "LV: Preference for VP intrinsics indicated. Will "
+                           "not generate VP intrinsics since interleave count "
----------------
It looks like there may not be a test for this code path?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5244
+
+      if (PreferVPIntrinsics)
+        MaxFactors.FixedVF = ElementCount::getFixed(1);
----------------
Would be good to document what is going on here (effectively disabling fixed with vectorization and why)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1415
+
+    // TODO: Add support for MaxSafeDist for correct loop emission.
+    Constant *WidthArg = State.Builder.getInt32(ElementWidth);
----------------
Does this mean the code we generate is not correct cases where `MaxSafeDist` needs to be handled?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1387
+         "Neither unrolling, nor interleaving is supported by RVV VLA");
+  State.Builder.SetInsertPoint(State.CFG.PrevBB->getFirstNonPHI());
+  for (unsigned Part = 0, UF = State.UF; Part < UF; ++Part) {
----------------
ABataev wrote:
> fhahn wrote:
> > Why does the insert point need resetting?
> Must be the very first instructions in the loop, all other instructions depend on this.
Can it be placed at the beginning and marked as having side-effects so it won't get moved?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750