[PATCH] D133955: [AArch64][CostModel] Add costs for fixed operations when using fixed vectors over SVE

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for the changes, this is moving in the right direction. I've left a few more small comments.



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2166
+      SrcTy.getVectorNumElements() == DstTy.getVectorNumElements() &&
+      ST->useSVEForFixedLengthVectors(WiderTy)) {
+    std::pair<InstructionCost, MVT> LT =
----------------
nit: `useSVEForFixedLengthVectors` implies `ST->hasSVE()` so you can remove `ST->hasSVE()`.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2170
+    unsigned NumElements =
+        128 / LT.second.getVectorElementType().getSizeInBits();
+    return AdjustCost(
----------------
Like I mentioned in my other comment, please use `AArch64::SVEBitsPerBlock` instead of hardcoding 128.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2175
+            Opcode,
+            ScalableVectorType::get(cast<VectorType>(Dst)->getElementType(),
+                                    NumElements),
----------------
nit: you can just write: `Dst->getScalarType()`
see https://llvm.org/doxygen/classllvm_1_1Type.html#af60c6120dbc5acdfcff9a7621d123796


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2177
+                                    NumElements),
+            ScalableVectorType::get(cast<VectorType>(Src)->getElementType(),
+                                    NumElements),
----------------
nit: you can just write: `Src->getScalarType()`


================
Comment at: llvm/test/Analysis/CostModel/AArch64/cast.ll:5
+; RUN: opt -passes="print<cost-model>" 2>&1 -disable-output -mtriple=aarch64-none-linux-gnueabi -mattr=+sve -aarch64-sve-vector-bits-min=256 %s | FileCheck --check-prefixes=SVE,FIXED-OVER-SVE256 %s
+; RUN: opt -passes="print<cost-model>" 2>&1 -disable-output -mtriple=aarch64-none-linux-gnueabi -mattr=+sve -aarch64-sve-vector-bits-min=2048 %s | FileCheck --check-prefixes=SVE,FIXED-OVER-SVE2048 %s
+; RUN: opt -passes="print<cost-model>" 2>&1 -disable-output -mtriple=aarch64-none-linux-gnueabi -mattr=+sve -force-streaming-compatible-sve -aarch64-sve-vector-bits-min=128 %s | FileCheck --check-prefixes=SVE,SVE128-NO-NEON %s
----------------
Does this RUN line add much value beyond the RUN line above for 256 bits? If not, can you remove it?


================
Comment at: llvm/test/Analysis/CostModel/AArch64/cast.ll:5
+; RUN: opt -passes="print<cost-model>" 2>&1 -disable-output -mtriple=aarch64-none-linux-gnueabi -mattr=+sve -aarch64-sve-vector-bits-min=256 %s | FileCheck --check-prefixes=SVE,FIXED-OVER-SVE256 %s
+; RUN: opt -passes="print<cost-model>" 2>&1 -disable-output -mtriple=aarch64-none-linux-gnueabi -mattr=+sve -aarch64-sve-vector-bits-min=2048 %s | FileCheck --check-prefixes=SVE,FIXED-OVER-SVE2048 %s
+; RUN: opt -passes="print<cost-model>" 2>&1 -disable-output -mtriple=aarch64-none-linux-gnueabi -mattr=+sve -force-streaming-compatible-sve -aarch64-sve-vector-bits-min=128 %s | FileCheck --check-prefixes=SVE,SVE128-NO-NEON %s
----------------
sdesmalen wrote:
> Does this RUN line add much value beyond the RUN line above for 256 bits? If not, can you remove it?
nit: s/FIXED-OVER/FIXED-MIN-256/ ?


================
Comment at: llvm/test/Analysis/CostModel/AArch64/cast.ll:6
+; RUN: opt -passes="print<cost-model>" 2>&1 -disable-output -mtriple=aarch64-none-linux-gnueabi -mattr=+sve -aarch64-sve-vector-bits-min=2048 %s | FileCheck --check-prefixes=SVE,FIXED-OVER-SVE2048 %s
+; RUN: opt -passes="print<cost-model>" 2>&1 -disable-output -mtriple=aarch64-none-linux-gnueabi -mattr=+sve -force-streaming-compatible-sve -aarch64-sve-vector-bits-min=128 %s | FileCheck --check-prefixes=SVE,SVE128-NO-NEON %s
 
----------------
nit: you can remove this flag, because 128bits is the minimum by default.


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

https://reviews.llvm.org/D133955