[PATCH] D90950: [AArch64]Add memory op cost model for SVE

[Caroline via Phabricator via llvm-commits]

CarolineConcatto marked 6 inline comments as done.
CarolineConcatto added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:754
+bool AArch64TTIImpl::useNeonVector(Type *Ty) {
+  return !(cast<VectorType>(Ty)->getElementCount().isScalable() ||
+           ST->useSVEForFixedLengthVectors());
----------------
sdesmalen wrote:
> You can write `isa<FixedVectorType>(Ty)` instead of `!cast<VectorType>(Ty)->getElementCount().isScalable()`.
> 
> Maybe this is personal preference, but it reads simpler to me if you rewrite the boolean expression to:
>   return isa<FixedVectorType>(Ty) && !ST->useSVEForFixedLengthVectors();
Thank you for the suggestion. I agree that is clear when working with boolean expression.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:785
 
   if (Ty->isVectorTy() &&
+      cast<VectorType>(Ty)->getElementType()->isIntegerTy(8) &&
----------------
sdesmalen wrote:
> `useNeonVector` should only return `true` if `Ty` is a vector, so you can simplify this expression to:
>   if (useNeonVector(Ty) &&
>      cast<VectorType>(Ty)->getElementType()->isIntegerTy()) {
>     ...
>   }
I think this is fixed.
Just in case, I believe the 8 inside cast<VectorType>(Ty)->getElementType()->isIntegerTy() is not intentional


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h:150
                                                     bool IsZeroCmp) const;
+  bool useNeonVector(Type *Ty);
 
----------------
sdesmalen wrote:
> Missing `const` for Type (because it doesn't modify `Ty`) and the method itself (because it doesn't modify the TargetTransform object), e.g.
> 
>   bool useNeonVector(const Type *Ty) const;
Yes, indeed. Missed that, thank you Sander.
I believe it is fixed now.


================
Comment at: llvm/test/Analysis/CostModel/AArch64/mem-op-cost-model.ll:51
+; CHECK-NEON: Cost Model: Found an estimated cost of 64 for instruction:
+; CHECK-SVE: Cost Model: Found an estimated cost of 64 for instruction:
+; CHECK-SVE-256: Cost Model: Found an estimated cost of 1 for instruction:
----------------
rengolin wrote:
> I'm assuming that the idea is that this vector will be NEON and not SVE, so the cost is the same.
Hi @rengolin you are correct.
I added an explanation at the beginning of the test.
According to the test //useSVEForFixedLengthVectors()// all fixed vector width lower than 256 bits will  neon isa instead of sve, so the performance for CHECK-NEON and CHECK-SVE-128 should be the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90950