[PATCH] D132477: Improve cost model for some 128-bit vector operations that use SVE

[Hassnaa Hamdi via Phabricator via llvm-commits]

hassnaa-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1995-1997
+  bool OverrideNEON =
+      (supportsScalableVectors() &&
+       ((LT.second == MVT::v2i64) || (LT.second == MVT::v4i32)));
----------------
sdesmalen wrote:
> This should be checking whether we can use SVE for the specified type (Ty). That means to check we have SVE, instead of checking whether the target supports scalable vectors. The answer will be the same, but conceptually we're specifically checking if we can use the SVE div instructions explicitly.
> 
> Note that SVE also supports it for larger vectors (if the SVE min bits allows) and for vectors with FP elements.
"Note that SVE also supports it for larger vectors (if the SVE min bits allows) and for vectors with FP elements." 
Do you mean that I should check for larger vectors ?
I checked for only v2i64 and v4i32 because that was specified in the jira ticket.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2049
+        InstructionCost OpCost = (IsFloat ? 2 : 1);
+        // multiply by 2 because it's calculated for both extract and insert
+        Cost += (LT.first * OpCost * 2);
----------------
sdesmalen wrote:
> I wouldn't expect there to be any extract and insert, because the division is legal.
I found that the code flow calculate the cost for both extract and insert, that's why I considered them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132477