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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

I spotted a few more things I missed in the last review, but I'm nearly happy.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1826
+InstructionCost AArch64TTIImpl::getSpliceCost(VectorType *Tp, int Index) {
+
+  static const CostTblEntry ShuffleTbl[] = {
----------------
nit: redundant whitespace.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1848
+                   getPromotedTypeForPredicate(LT.second) : LT.second;
+  Type *Promoted = EVT(PromotedVT).getTypeForEVT(Tp->getContext());
+  InstructionCost LegalizationCost = 0;
----------------
nit: `PromotedTy`


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1852
+    LegalizationCost =
+        getCmpSelInstrCost(Instruction::ICmp, LegalVTy, LegalVTy,
+                           CmpInst::BAD_ICMP_PREDICATE, CostKind) +
----------------
I'm not sure if this matters, but for LegalVTy that's e.g. nxv16i8, the CondTy is nxv16i1, not LegalVTy.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1854
+                           CmpInst::BAD_ICMP_PREDICATE, CostKind) +
+        getCmpSelInstrCost(Instruction::Select, LegalVTy, LegalVTy,
+                           CmpInst::BAD_ICMP_PREDICATE, CostKind);
----------------
Should these two selects also be performed on `Promoted`?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1856
+                           CmpInst::BAD_ICMP_PREDICATE, CostKind);
+  }
+  if (LT.second.getScalarType() == MVT::i1) {
----------------
nit: add newline after this, and maybe add a one line comment saying that this implements the cost of the operation being performed on a promoted type.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1811
+InstructionCost AArch64TTIImpl::getSpliceCost(VectorType *Tp, int Index) {
+  auto getPromotedTypeForPredicate = [&](MVT M) {
+    switch (M.SimpleTy) {
----------------
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104630