[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 struggling with how best to represent STEPVECTOR's scale operand.  The patch as it currently stands is probably good enough but I do wonder how long we'll get by without needing to implement PromoteIntOp_STEP_VECTOR.  I also wonder if we can take a similar approach as done for INSERT_SUBVECTOR's index operand.

The description for how STEPVECTOR handles overflow also seems a bit limiting.  Specifically this patch lowers vector of i1 based STEP_VECTOR operations, but with the current description only the first two lanes of any vector type will result in defined behaviour, which makes me wonder the value in allowing such support.



================
Comment at: llvm/include/llvm/CodeGen/SelectionDAG.h:843
 
+  /// Returns an ISD::STEP_VECTOR node, which is a vector of type ResVT whose
+  /// elements contain the linear sequence <0, Step, Step * 2, Step * 3, ...>
----------------
This is no longer true, perhaps just `Returns a vector of type...`


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:1727
+  SmallVector<SDValue, 16> OpsStepConstants;
+  for (uint64_t i = 0; i < ResVT.getVectorMinNumElements(); i++)
+    OpsStepConstants.push_back(getConstant(StepVal * i, DL, OpVT));
----------------
Given this is fixed length specific I think calling `getVectorNumElements()` is clearer.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:4294
+  if (cast<ConstantSDNode>(Step)->isNullValue())
+    // We can just reuse Step here as we need a zero value anyway
+    return DAG.getConstant(0, DL, VT);
----------------
Comment is no longer valid.  I'd just remove it.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:4641
+    assert(isa<ConstantSDNode>(Operand) &&
+           !cast<ConstantSDNode>(Operand)->getAPIntValue().isNegative() &&
+           "Expected positive integer constant for STEP_VECTOR");
----------------
FYI: APInt has a function for this called `isNonNegative()`.


================
Comment at: llvm/lib/IR/IRBuilder.cpp:95
+Value *IRBuilderBase::CreateStepVector(Type *DstType, const Twine &Name) {
+  if (isa<FixedVectorType>(DstType)) {
+    Type *STy = DstType->getScalarType();
----------------
Keep this style if you prefer but I think it will be cleaner done as
```
if  (isa<ScalableVectorType>(DstType))
  return CreateIntrinsic(Intrinsic::experimental_stepvector, {DstType}, {},....

<fixed vector handling here>



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:264-265
+  case Intrinsic::experimental_stepvector: {
+    assert(isa<VectorType>(RetTy) &&
+           cast<VectorType>(RetTy)->getElementType()->isIntegerTy());
+    unsigned Cost = 1; // Cost of the `index' instruction
----------------
This is safe to assume and already checked within Verifier.cpp.


================
Comment at: llvm/test/CodeGen/AArch64/neon-stepvector.ll:9-10
+; CHECK:       // %bb.0: // %entry
+; CHECK-NEXT:    adrp x8, .LCPI0_0
+; CHECK-NEXT:    ldr q0, [x8, :lo12:.LCPI0_0]
+; CHECK-NEXT:    ret
----------------
For this test to be correct requires LCPI0_0 to point to something meaningful.  I believe update_llc_test_checks.py strips this information so you'll need to manually add it.  We did likewise for named-vector-shuffle-reverse-neon.ll. 


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

https://reviews.llvm.org/D97299