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

[David Sherwood via Phabricator via llvm-commits]

david-arm accepted this revision.
david-arm added a comment.

LGTM with nits addressed. Thanks @hassnaa-arm!



================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:2088
+      if (TLI->isOperationLegalOrCustom(ISD, LT.second) && ST->hasSVE()) {
+        // SDIV/UDIV operations are lowered, then we can have less costs.
+        if (isa<FixedVectorType>(Ty) &&
----------------
nit: I think the comment is still a bit vague here because the operations are always lowered somehow, whether they are marked as legal, custom or expand. I think the key thing is *how* they are lowered. I think it's important to mention they are `lowered using SVE`. Can you just add something like that before landing the patch?


================
Comment at: llvm/test/Analysis/CostModel/AArch64/sve-fixed-length.ll:63
+; Assuming legalization_cost = (vec_len-1/VBITS)+1
+; Assuming extra cost of 8 for i8.
+; Assuming extra cost of 4 for i16.
----------------
nit: This still says `Assuming extra cost of 8`, which implies we're **adding ** a cost of 8, not multiplying. Before landing the patch can you change this to say we're multiplying the cost by 8?


================
Comment at: llvm/test/Analysis/CostModel/AArch64/sve-fixed-length.ll:65
+; Assuming extra cost of 4 for i16.
+; The hard-coded expected cost is based on VBITS=128
+define void @sdiv() #0 {
----------------
nit: I think this comment can be removed now.


================
Comment at: llvm/test/Analysis/CostModel/AArch64/sve-fixed-length.ll:115
+; Assuming extra cost of 4 for i16.
+; The hard-coded expected cost is based on VBITS=128
+define void @udiv() #0 {
----------------
nit: This comment can also be removed now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132477