[PATCH] D99750: [LV, VP]VP intrinsics support for the Loop Vectorizer

[Alexey Bataev via Phabricator via llvm-commits]

ABataev marked 13 inline comments as done.
ABataev added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h:84
+  bool hasActiveVectorLength(unsigned Opcode, Type *DataType,
+                             Align Alignment) const;
+
----------------
fhahn wrote:
> ABataev wrote:
> > Ayal wrote:
> > > Better called `hasEffectiveVectorLength()` as in hasEVL - for consistency?
> > > From the summary: "We use active vector length and explicit vector length interchangeably" - why? Could only EVL be used, consistently? (RISC-V documentation of setvl has AVL as a parameter and [E]VL as return/set value.)
> > > 
> > > Add Opcode/DataType/Alignment parameters later - when used?
> > This is separate. This function is already introduced in TTI, the purpose of the patch is not related to TTI directly.
> > As to params, it is done in the implementation.
> Just following up on this, should this the name be changed in TTI? Do you know the reason for referring to it has active vector length there vs effective vector length in the patch?
We can do it later in a separate patch. EVL stands for Explicit vector length, TTI interface was introduced long before this patch. There are just different abbrevs for the same technique - Active Vector Length, Explicit Vector length, etc.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9634
         Value *VectorGep = State.get(getAddr(), Part);
         NewSI = Builder.CreateMaskedScatter(StoredVal, VectorGep, Alignment,
                                             MaskPart);
----------------
fhahn wrote:
> Is the gather scatter case handled correctly for EVL at the moment?
Added support for this.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:371
+           "EVL type must be smaller than Phi type.");
+    EVL = Builder.CreateIntCast(EVL, Phi->getType(), /*isSigned=*/false);
+    return Builder.CreateAdd(Phi, EVL, Name, hasNoUnsignedWrap(),
----------------
fhahn wrote:
> might be good to have a test case where the induction is `i32` and no cast is needed.
Added @iv32 function in the test


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:210
+  const VPlan *Plan = VPBB->getPlan();
+  if (Plan && Plan->getEntry()->getNumSuccessors() == 1) {
+    Header = Plan->getVectorLoopRegion()->getEntry();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750