[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() &&
----------------
You shouldn't need this, my guess is that you've git the case when it returns 0 to signify the minimum is unknown.  In which case we typically use `unsigned MinSVEVectorSize = std::max(ST->getMinSVEVectorSizeInBits(), 128u);`


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1932-1934
+    EVT WiderTy = (SrcTy.getFixedSizeInBits() > DstTy.getFixedSizeInBits())
+                      ? SrcTy
+                      : DstTy;
----------------
What about `EVT WiderTy = SrcTy.bitsGT(DstTy) ? SrcTy : DstTy;`?


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1938-1942
+      const unsigned NumElements =
+          ((WiderTy.getFixedSizeInBits() / ST->getMinSVEVectorSizeInBits()) <=
+           1)
+              ? 128 / WiderTy.getVectorElementType().getSizeInBits()
+              : WiderTy.getVectorNumElements();
----------------
I think you've misunderstood my previous comment as now we're back to missing the conversion from fixed length num elts to scalable num elts.  I know I've mentioned this before and it seemed a dead end but I going to ask again.  Why can we not use the existing legalisation cost functions?  I'm think something like:

```
std::pair<InstructionCost, MVT> LT = getTypeLegalizationCost(WiderType);
if (LT.second.getFixedSizeInBits() > 128 || ST->forceStreamingCompatibleSVE()) {                                                      
      unsigned NumElements = 128 / LT.second.getVectorElementType().getSizeInBits();

....

return AdjustCost(LT.first * getCastInstrCost(....
```
Doing this means we don't need to worry about `getMinSVEVectorSizeInBits`.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1946
+          Opcode,
+          VectorType::get(cast<VectorType>(Dst)->getElementType(), NumElements,
+                          true),
----------------
Can you be specific and use `ScalableVectorType::get()` here.


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

https://reviews.llvm.org/D133955