[PATCH] D97299: [IR][SVE] Add new llvm.experimental.stepvector intrinsic

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

I'm still working through the patch but here's what I've got for now.



================
Comment at: llvm/docs/LangRef.rst:16282
+used for scalable vectors with integer elements. The intrinsic also works for
+fixed width vectors, but the user will probably get better code quality
+generating constant vectors instead. If the sequence value exceeds the allowed
----------------
To be consistent with the other experimental vector intrinsics this should be `fixed-width`?


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:1253-1254
+    case Intrinsic::experimental_stepvector: {
+      assert(RetTy->isIntegerTy() &&
+             "Stepvector can only be used for integer vectors");
+      if (isa<ScalableVectorType>(RetTy))
----------------
This seems like the wrong place to validate the intrinsic.


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAG.h:657-658
 
+  SDValue getStepVector(const SDLoc &DL, EVT ResVT, EVT OpVT,
+                        const APInt Step) {
+    assert(Step.getActiveBits() <= OpVT.getScalarSizeInBits());
----------------
I guess I've caused this with my previous "don't use a node request" and thus the answer will be no, but I'll ask the question anyway. Can OpVT be dropped here? as it seems inconvenient.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:1637-1643
+  EVT EltVT = Step.getValueType();
+  SDValue EltCnt = DAG.getVScale(
+      dl, EltVT,
+      APInt(EltVT.getScalarSizeInBits(), LoVT.getVectorMinNumElements()));
+  SDValue StartOfHi = DAG.getNode(ISD::MUL, dl, EltVT, EltCnt, Step);
+  StartOfHi = DAG.getZExtOrTrunc(StartOfHi, dl, HiVT.getVectorElementType());
+  StartOfHi = DAG.getNode(ISD::SPLAT_VECTOR, dl, HiVT, StartOfHi);
----------------
The Step is an immediate, as is the known part of LoVT's element count, which suggests you shouldn't need to use DAG for the maths because you can do `getVScale(N->getConstantOperandAPInt(0) * LoVT.getVectorMinNumElements())`


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:4277
+    // We can just reuse Step here as we need a zero value anyway
+    return DAG.getNode(ISD::SPLAT_VECTOR, DL, VT, Step);
+
----------------
I think `DAG.getConstant(0...` is better here so that the target's preferred nodes (i.e. SPLAT_VECTOR or BUILD_VECTOR) are used.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:10847-10848
+  EVT ResultVT = TLI.getValueType(DAG.getDataLayout(), I.getType());
+  assert(ResultVT.isVector() && ResultVT.isInteger() &&
+         "Only expect integer vector types used for experimental_stepvector");
+  EVT OpVT =
----------------
Is this needed? I ask because `DAG.getStepVector` will validate the result VT in one place rather than expecting all it's users to do likewise.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:10855-10858
+    SmallVector<SDValue, 16> OpsStepConstants;
+    for (uint64_t i = 0; i < ResultVT.getVectorMinNumElements(); i++)
+      OpsStepConstants.push_back(DAG.getConstant(i, DL, OpVT));
+    setValue(&I, DAG.getBuildVector(ResultVT, DL, OpsStepConstants));
----------------
What about pushing this into `DAG.getStepVector`? that way there's a generic way for any code to create this vector without needing to worry about the result type.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:272-273
+      Type *LegalVTy = EVT(LT.second).getTypeForEVT(RetTy->getContext());
+      Cost += (LT.first - 1) *
+              getArithmeticInstrCost(Instruction::Add, LegalVTy, CostKind);
+    }
----------------
In a world where InstructionCost is everywhere I think it's better for the InstructionCost side to be the LHS because it reduces the number of required operator overloads.  Check with @sdesmalen but I think you'll save some work if you write this as `getArithmeticInstrCost() * (LT.first - 1)`.


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

https://reviews.llvm.org/D97299