[PATCH] D95598: [AArch64][SVE]Add cost model for broadcast shuffle

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1241
+      { TTI::SK_Broadcast, MVT::nxv2i64, 1 },
+      { TTI::SK_Broadcast, MVT::nxv8f16, 1 },
+      { TTI::SK_Broadcast, MVT::nxv4f32, 1 },
----------------
I think we also need one for the type MVT::nxv8bf16 here.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:1241
   }
-
   return BaseT::getShuffleCost(Kind, Tp, Index, SubTp);
----------------
david-arm wrote:
> I think we also need one for the type MVT::nxv8bf16 here.
nit: Whitespace change


================
Comment at: llvm/test/Analysis/CostModel/AArch64/sve-shuffle-broadcast.ll:11
+
+define <vscale x 16 x i8> @broadcast_v16i8(<vscale x 16 x i8> %v) {
+; CHECK-LABEL: 'broadcast_v16i8':
----------------
Hi Carol, this is just a suggestion, but a reviewer (David Green) on one of my recent cost model patches suggested that we could do all tests in one function, i.e.

define void @broadcast_legal() {
%0 = shufflevector <vscale x 16 x i8> %v, <vscale x 16 x i8> poison, <vscale x 16 x i32> zeroinitializer
%1 = shufflevector <vscale x 32 x i8> %v, <vscale x 32 x i8> poison, <vscale x 32 x i32> zeroinitializer
...
}

and so on. I found it really useful as all the CHECK lines can go in the same function and it reduces the test size. He mentioned that in my patch here - https://reviews.llvm.org/D95039. It means we can reduce the number of functions and number of unnecessary checks for the "ret void" instruction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95598