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

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

This is looking better, thanks @hassnaa-arm! I still have a few more comments about the cost model and the tests, but it's almost there. :)



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2043
+        // require extra work :
+        if (LT.second == MVT::v16i8)
+          Cost *= 8;
----------------
Hi @hassnaa-arm, I think we should be checking for the element type here rather than the full VT because that also covers other types such as MVT::v8i8, etc.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2045
+          Cost *= 8;
+        if (LT.second == MVT::v8i16)
+          Cost *= 4;
----------------
Same thing here - we should be checking the element type only.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2076
+    // the SVE mul instruction, which has a lower cost.
+    if (LT.second == MVT::v2i64 &&
+        (TLI->isOperationLegalOrCustom(ISD, LT.second)))
----------------
It's ok to check the specific MVT::v2i64 here because we don't care about v1i64.


================
Comment at: llvm/test/Analysis/CostModel/AArch64/sve-fixed-length.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py
 ; RUN: opt < %s -passes="print<cost-model>" 2>&1 -disable-output | FileCheck %s -D#VBITS=128
----------------
nit: Could you remove this NOTE please? It's just a bit misleading that's all because if you do actually run that script on this file it deletes the CHECK lines for the `@add()` function.


================
Comment at: llvm/test/Analysis/CostModel/AArch64/sve-fixed-length.ll:137
+entry:
+  %mul512.i64 = udiv <8 x i64> undef, undef
+
----------------
I think this should be `mul <8 x i64> undef, undef`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132477