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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:753
 }
+bool AArch64TTIImpl::useNeonVector(Type *Ty) {
+  return !(cast<VectorType>(Ty)->getElementCount().isScalable() ||
----------------
nit: add whitespace above.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:754
+bool AArch64TTIImpl::useNeonVector(Type *Ty) {
+  return !(cast<VectorType>(Ty)->getElementCount().isScalable() ||
+           ST->useSVEForFixedLengthVectors());
----------------
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();


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:785
 
   if (Ty->isVectorTy() &&
+      cast<VectorType>(Ty)->getElementType()->isIntegerTy(8) &&
----------------
`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()) {
    ...
  }


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h:150
                                                     bool IsZeroCmp) const;
+  bool useNeonVector(Type *Ty);
 
----------------
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;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90950