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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:8961-8962
+  EVT ElemVT = VT.getScalarType();
+  if (ElemVT != MVT::i1)
+    return SDValue();
+
----------------
david-arm wrote:
> paulwalker-arm wrote:
> > 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())`
> Hi @paulwalker-arm, me too for i1 case, however I tried the generic route first exactly as you suggest and it didn't work. The problem was that if I created the ISD::TRUNC node we never ended up custom lowering as you'd expect. If you look at AArch64TargetLowering::LowerTRUNCATE for i1 element types we return this:
> 
> ```return DAG.getSetCC(dl, VT, And, Zero, ISD::SETNE);```
> 
> which then never gets into our custom lowering code, despite the action being set to Custom for precisely those types. We then crash in isel trying to match a `setcc` operation. However I'd love to do it this way if you've got any suggestions for how I can solve this problem? I suspect there are just too many levels of custom lowering involved, i.e. custom lower STEP_VECTOR, followed by custom lower TRUNCATE, followed by custom lower SETCC.
> 
> So in general your preference is to custom lower *all* types and go through this function to create AArch64ISD::INDEX_VECTOR?
One solution might be to rewrite LowerTRUNCATE to avoid returning SETCC for scalable vectors and use AArch64ISD::SETCC_MERGE_ZERO directly? I'd just assumed people would find this a bit ugly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97299