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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4759
+
+  return DAG.getNode(ISD::STEP_VECTOR, dl, NOutVT, NewStep);
+}
----------------
Given that we mandate the Step must be an immediate, I wondering if DAG should have a getStepVector function that takes such.  I say this because, for example, what are you expecting to happen if `DAG.getNode(ISD::ANY_EXTEND` did not get folded away.

As an aside, do you really not care about the type of extension?  I think you absolutely need zero extension based on the current node restrictions.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:1634-1636
+  SDValue EltCnt = DAG.getVScale(
+      dl, EltVT,
+      APInt(EltVT.getScalarSizeInBits(), LoVT.getVectorMinNumElements()));
----------------
I guess there's no action required for this patch as fixed length vectors are currently excluded, however, if this idiom becomes common then I suggest creating a DAG.getElementCountAsNode(EVT) like function.  That way code like this will just work for both fixed and scalable vectors.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:4269-4271
+  const APInt &StepVal = cast<ConstantSDNode>(Step)->getAPIntValue();
+
+  if (StepVal == 0)
----------------
`if (cast<ConstantSDNode>(Step)->isNullValue())` ?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:4615
+    assert(isa<ConstantSDNode>(Operand) &&
+           !cast<ConstantSDNode>(Operand)->getAPIntValue().isNegative() &&
+           "Expected positive integer constant for STEP_VECTOR");
----------------
Is it worth also validating that Operand is at least as big as the result element type. That way if the "not-negative" requirement ever get's dropped I think the signedness will not actually matter?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6875
+  case Intrinsic::experimental_stepvector: {
+    auto DL = getCurSDLoc();
+    EVT ResultVT = TLI.getValueType(DAG.getDataLayout(), I.getType());
----------------
Perhaps worth punting this into visitStepVector.  This is what we've done for vector.reverse.


================
Comment at: llvm/lib/IR/IRBuilder.cpp:97
+    Type *STy = DstType->getScalarType();
+    ElementCount EC = cast<FixedVectorType>(DstType)->getElementCount();
+
----------------
Given this is a FixedVectorType you should be able to short cut ElementCount and use getNumElements() directly.


================
Comment at: llvm/lib/IR/IRBuilder.cpp:105-107
+    Constant *Cv = ConstantVector::get(Indices);
+    assert(Cv->getType() == DstType && "Invalid consecutive vec");
+    return Cv;
----------------
I cannot see how this assert will ever fire and so `return ConstantVector::get(Indices);` just looks nicer.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:8961-8962
+  EVT ElemVT = VT.getScalarType();
+  if (ElemVT != MVT::i1)
+    return SDValue();
+
----------------
I'm not a fan of the pattern duplication within AArch64SVEInstrInfo.td so I'm thinking we should lower all ISD::STEP_VECTORs to AArch64ISD::INDEX_VECTOR.

Regardless of whether you do that I would expect the predicate handling to be more generic, for example:
`return ISD::TRUNC(ISD::STEP_VECTOR(Op.getConstantOperandVal())`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97299