[PATCH] D120880: [AArch64] Insert subvector costs

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2716
+  // move.
+  if (Kind == TTI::SK_InsertSubvector && !isa<ScalableVectorType>(Tp) &&
+      LT.second.isVector() && SubTp) {
----------------
nit: It seems more natural to write `isa<FixedVectorType>(Tp)`


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2717
+  if (Kind == TTI::SK_InsertSubvector && !isa<ScalableVectorType>(Tp) &&
+      LT.second.isVector() && SubTp) {
+    int NumElts = LT.second.getVectorNumElements();
----------------
If you check this is a fixed-width vector using `LT.second.isFixedLengthVector()`, you can remove the test for `Tp` being a fixed-length vector above.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2718
+      LT.second.isVector() && SubTp) {
+    int NumElts = LT.second.getVectorNumElements();
+    std::pair<InstructionCost, MVT> SubLT =
----------------
nit: move closer to use.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2721
+        TLI->getTypeLegalizationCost(DL, SubTp);
+    if (SubLT.second.isVector()) {
+      int NumSubElts = SubLT.second.getVectorNumElements();
----------------
I think this code silently assumes fixed-width == NEON, so I think it's worth adding a check that vector type being inserted into is less than 128bits. (and something similar for the value of the index being within a certain range). For fixed-length SVE vectors where wider vectors are legal, these ranges may go beyond 128bits.

It would also be useful to have a test for that case.


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

https://reviews.llvm.org/D120880