[PATCH] D104630: [AArch64][CostModel] Add cost model for experimental.vector.splice

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:859
 
-
     // LowerVectorFP_TO_INT
----------------
nit: Whitespace


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1820
+  default:
+    return MVT::Untyped;
+  }
----------------
I think at the point we call this function the type has been legalised and split into LT.first (a multiple of a legal type) and LT.second (a legal type). So I think I'd expect the default case to be unreachable here perhaps?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1861
+  if (LT.second.getScalarType() == MVT::i1) {
+    LegalizationCost += getCastInstrCost(Instruction::ZExt, PromotedVTy, LegalVTy,
+                                         TTI::CastContextHint::None, CostKind) +
----------------
nit: Can you fix the formatting here please? Thanks!


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1811
+InstructionCost AArch64TTIImpl::getSpliceCost(VectorType *Tp, int Index) {
+  auto getPromotedTypeForPredicate = [&](MVT M) {
+    switch (M.SimpleTy) {
----------------
sdesmalen wrote:
> CarolineConcatto wrote:
> > sdesmalen wrote:
> > > Can you move this out of the function into a separate `static MVT getPromotedTypeForPredicate` function? Perhaps we'll want to reuse this at a later point.
> > Is this what you were suggesting?
> it was, thanks!
Just FYI there is actually already a function in AArch64ISelLowering.cpp that does something very similar:

```
static inline EVT getPromotedVTForPredicate(EVT VT) {
  assert(VT.isScalableVector() && (VT.getVectorElementType() == MVT::i1) &&
         "Expected scalable predicate vector type!");
  switch (VT.getVectorMinNumElements()) {
  default:
    llvm_unreachable("unexpected element count for vector");
  case 2:
    return MVT::nxv2i64;
  case 4:
    return MVT::nxv4i32;
  case 8:
    return MVT::nxv8i16;
  case 16:
    return MVT::nxv16i8;
  }
}
```

I wonder if it's worth having a common routine in a header file?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104630