[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/VPlanRecipes.cpp:1743
+  PHINode *EntryPart =
+      State.Builder.CreatePHI(Start->getType(), 2, "evl.based.iv");
+  EntryPart->addIncoming(Start, VectorPH);
----------------
ABataev wrote:
> shiva0217 wrote:
> > There is a case that the PHI didnt' been inserted at top of basic block.
> > 
> >     int foo (int value, int *buf, int *end) {
> >       int *tmp;
> >       for (tmp = buf; tmp < end; tmp++)
> >         value -= *tmp;
> >       return value;
> >     }
> > 
> > Should we specify insertion point?
> > Something like:
> > 
> >     PHINode *EntryPart = PHINode::Create(
> >         Start->getType(), 2, "evl.based.iv", &*State.CFG.PrevBB->getFirstInsertionPt());
> Fixed in VPlanTransforms.cpp by inserting the recipe immediately after CanonicalIVPHI.
I think `VPEVLBasedIVPHIRecipe` should be turned into a subclass of VPHeaderPHIRecipe, this will also ensure that the VPlan verifier checks it is in the header section


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:986
+  auto *WideCanonicalIV =
+      cast<VPWidenCanonicalIVRecipe>(*FoundWidenCanonicalIVUser);
+  // Walk users of WideCanonicalIV and replace all compares of the form
----------------
ABataev wrote:
> shiva0217 wrote:
> > There is a case that VPWidenCanonicalIVRecipe didn't be generated with tail folding.
> > 
> >     int i;
> >     int foo (int q, int z)
> >     {
> >       int e = 0;
> >       while (z < 1)
> >         {
> >           q = z * 2;
> >           if (q != 0)
> >             for (i = 0; i < 2; ++i)
> >               e = 5;
> >           ++z;
> >         }
> >       return e;
> >     }
> > 
> > `for (i = 0; i < 2; ++i)` been simplifed as `store i32 2, ptr @i`.
> > Both pointer and store value are loop-invariant, so the mask(VPWidenCanonicalIVRecipe) might not be generated.
> > Should we suppress the replacement when the mask is not available?
> Fixed, added the test
Was this fixed by adding the ` bool KeepVPCanonicalWidenRecipes` flag? What's the test case for this? There's a new `no_masking` case, but it has an empty body and no vector code is generated? 



================
Comment at: llvm/test/Transforms/LoopVectorize/RISCV/vectorize-vp-intrinsics.ll:5
+; RUN: -prefer-predicate-over-epilogue=predicate-dont-vectorize \
+; RUN: -mtriple=riscv64 -mattr=+v -S < %s 2>&1 | FileCheck --check-prefix=IF-EVL %s
+
----------------
no need to redirect stderr here?


================
Comment at: llvm/test/Transforms/LoopVectorize/RISCV/vectorize-vp-intrinsics.ll:7
+
+; RUN: opt -passes=loop-vectorize \
+; RUN: -prefer-predicate-with-vp-intrinsics=force-explicit-vector-length-support \
----------------
Can this configuration be used for target-independent tests?


================
Comment at: llvm/test/Transforms/LoopVectorize/RISCV/vectorize-vp-intrinsics.ll:385
+
+define void @masked_loadstore(ptr noalias %a, ptr noalias %b, i64 %n) {
+; IF-EVL-LABEL: @masked_loadstore(
----------------
This test file is getting quite big with 3 different run lines. I think it would be good to try to split this up a bit, to make it easier to see what's going on.

I'd recommend having the test cases for various legality issues as target-independent tests with force flags (force EVL support, VF and IC). And keep cost-model specific tests target specific.


================
Comment at: llvm/test/Transforms/LoopVectorize/RISCV/vplan-vp-intrinsics.ll:3
+
+; RUN: opt -passes=loop-vectorize -debug-only=loop-vectorize \
+; RUN: -prefer-predicate-with-vp-intrinsics=if-explicit-vector-length-support \
----------------
Can this test be target independent? does it need to check the no VP case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750