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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1929
+  // for a fixed type to be represented upon SVE registers.
+  if (ST->hasSVE() && ST->getMinSVEVectorSizeInBits() &&
+      SrcTy.isFixedLengthVector() && DstTy.isFixedLengthVector() &&
----------------
With my suggestion below (the useSVEForFixedLengthVectors one) I'm hoping you don't need these conditions?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1931
+      SrcTy.isFixedLengthVector() && DstTy.isFixedLengthVector() &&
+      SrcTy != DstTy) {
+    EVT WiderTy = (SrcTy.getFixedSizeInBits() > DstTy.getFixedSizeInBits())
----------------
Is this necessary? If it is then I'd expect it to be covered by the following call to getCastInstrCost.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1935-1937
+    if (WiderTy.getFixedSizeInBits() > 128 ||
+        (ST->forceStreamingCompatibleSVE() &&
+         (WiderTy.is128BitVector() || WiderTy.is64BitVector()))) {
----------------
I think this can be simplified to just `((ST->useSVEForFixedLengthVectors() && WiderTy.getFixedSizeInBits() > 128) || ST->forceStreamingCompatibleSVE())` because when in streaming mode we must use SVE for all vector operations.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1938
+         (WiderTy.is128BitVector() || WiderTy.is64BitVector()))) {
+      const unsigned Multiplier = std::max<unsigned>(
+          1u, WiderTy.getFixedSizeInBits() / ST->getMinSVEVectorSizeInBits());
----------------
Rather than create a multiplier for the cost of one scalable vector equivalent, does creating a bigger scalable vector type not work.  That way we benefit from existing code that computes the cost of legalisation rather than second guessing it.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1950-1951
+          getCastInstrCost(Opcode,
+                           DstScalableType.getTypeForEVT(I->getContext()),
+                           SrcScalableType.getTypeForEVT(I->getContext()), CCH,
+                           CostKind, I));
----------------
Rather than create an `EVT` only to convert to a `Type*` can you create the LLVM type directly?


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

https://reviews.llvm.org/D133955