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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for the changes, this is looking better! Just left a few more nits.



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


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1847
+  InstructionCost LegalizationCost = 0;
+  bool IsPredicated = (TLI->getValueType(DL, Tp, true).getScalarType() == MVT::i1);
+  if (Index < 0) {
----------------
If above you write:

  std::pair<InstructionCost, MVT> LT = TLI->getTypeLegalizationCost(DL, Tp);
  MVT PromotedVT = LT.second.getScalarType() == MVT::i1 ? getPromotedTypeForPredicate(LT.second) : LT.second;

Then you can drop the IsPredicated and instead inline `PromotedVT.getScalarType() == MVT::i1` in the condition below.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1869
+                                     : LT.second);
+  assert (Entry && "Ilegal Type for Splice");
+  LegalizationCost += Entry->Cost;
----------------
s/Ilegal/Illegal/


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1871-1873
+  if (LT.first > 1)
+    return LegalizationCost * LT.first;
+  return LegalizationCost;
----------------
If LT.first is `unsigned`, the if-condition is redundant, you can write
  return LegalizationCost * LT.first;
directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104630