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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev marked 8 inline comments as done.
ABataev added a comment.

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

> Thanks for the updates! It would also be good to update the description so it reflects the current status.

Could you point which part should be updated?

> In terms of testing, would it be possible to have most tests target-independent (using `-prefer-predicate-with-vp-intrinsics=force-explicit-vector-length-support` ) and limit the RISCV-dependent tests to cost modeling/checking of defaults? That ensures that we have wide test coverage by default (e.g. if someone doesn't have RISCV as enabled target).

It already has the test for target-independent and target dependent stuff. I don't think it is good to remove target-specific tests.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1650
+    return PreferVPIntrinsics && foldTailByMasking() &&
+           Legal->getMaxSafeDepDistBytes() == -1U;
+  }
----------------
fhahn wrote:
> Looks like this needs a test and the TODO from the recipe should probably go here to make clear why there's this restriction for now
Will add a test


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9845
+    if (auto *IMask = dyn_cast<VPInstruction>(Mask))
+      if (IMask->getOpcode() == VPInstruction::ICmpULE)
+        return Builder.getTrueVector(EC);
----------------
fhahn wrote:
> It's not clear to me what the reasoning is here to simplify this to the all-true mask without checking the operands of the compare. If the compare can be simplified to the all-true mask, then this would be suitable to do as VPlan-to-VPlan simplification instead of during codegen.
Added extra check.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9880
+        // intrinsic.
+        if (EVLPart) {
+          auto *StoredValTy = cast<VectorType>(StoredVal->getType());
----------------
fhahn wrote:
> For correctness, would we also need special handling for interleave groups?
if foldTailByMasking() is true (for VP support it is true), interleave groups are invalidated.


================
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:
> ABataev wrote:
> > 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.
> Reading this back now, the check below is for loops requiring scalar epilogues, not necessarily multiple exits. Would be good to update the comment and also add a test to make sure this is captured. AFAICT this is something that the patch already handles and should be tested (e.g. a test with an interleave group that requires scalar epilogue)
Will add a test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750