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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

In D99750#4385263 <https://reviews.llvm.org/D99750#4385263>, @fhahn wrote:

> 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

Well, generally speaking, it is controlled by EVL. If you check that cases, where setMask(true) is used, the next argument is setEVL().
Anyway, ff there are any limitations for this, we can fix this later.



================
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.
----------------
fhahn wrote:
> Is there a test with multiple exits?
It currently not supported by this patch. This is just an inital patch, it won't handle all the corner cases.


================
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 "
----------------
fhahn wrote:
> It looks like there may not be a test for this code path?
I'll check what we can do about it.


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1415
+
+    // TODO: Add support for MaxSafeDist for correct loop emission.
+    Constant *WidthArg = State.Builder.getInt32(ElementWidth);
----------------
fhahn wrote:
> Does this mean the code we generate is not correct cases where `MaxSafeDist` needs to be handled?
Yes, as I mentioned above, this is just a first patch, some extra work is required to handle correctly all corner cases. Plus, MaxSafeDist analysis needs to be adjusted for EVL, since we can support non-power-of-2 value here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750