[PATCH] D106929: [AArch64][SVE][InstCombine] Unpack of a splat vector -> Scalar extend

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-opts-unpkhi-unpklo.ll:1
+; RUN: opt -S -instcombine < %s | FileCheck %s
+
----------------
Please use `utils/update_test_checks.py` to generate the CHECK lines.


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-opts-unpkhi-unpklo.ll:26
+  %splat = shufflevector <vscale x 8 x i16> %splat_insert, <vscale x 8 x i16> poison, <vscale x 8 x i32> zeroinitializer
+  %unpack = tail call <vscale x 4 x i32> @llvm.aarch64.sve.uunpkhi.nxv8i32
+               (<vscale x 8 x i16> %splat)
----------------
The test is called `uunpklo_splat` but the IR is calling `uunpkhi`.


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-opts-unpkhi-unpklo.ll:31-49
+define void @uunpk_splat(i16 %a) #0 {
+; CHECK-LABEL: @uunpk_splat(
+; CHECK: %1 = zext i16 %a to i32
+; CHECK: %.splatinsert = insertelement <vscale x 4 x i32> poison, i32 %1, i32 0
+; CHECK: %unpackhi = shufflevector <vscale x 4 x i32> %.splatinsert, <vscale x 4 x i32> poison, <vscale x 4 x i32> zeroinitializer
+; CHECK: %2 = zext i16 %a to i32
+; CHECK: %.splatinsert1 = insertelement <vscale x 4 x i32> poison, i32 %2, i32 0
----------------
This looks like a redundant test which is not testing anything that is not already covered by the separate unsigned hi/lo tests.


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-opts-unpkhi-unpklo.ll:72
+  %splat = shufflevector <vscale x 8 x i16> %splat_insert, <vscale x 8 x i16> poison, <vscale x 8 x i32> zeroinitializer
+  %unpack = tail call <vscale x 4 x i32> @llvm.aarch64.sve.sunpkhi.nxv8i32
+               (<vscale x 8 x i16> %splat)
----------------
The test is called `sunpklo_splat` but the IR is calling `sunpkhi`.


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-opts-unpkhi-unpklo.ll:77-95
+define void @sunpk_splat(i16 %a) #0 {
+; CHECK-LABEL: @sunpk_splat(
+; CHECK: %1 = sext i16 %a to i32
+; CHECK: %.splatinsert = insertelement <vscale x 4 x i32> poison, i32 %1, i32 0
+; CHECK: %unpackhi = shufflevector <vscale x 4 x i32> %.splatinsert, <vscale x 4 x i32> poison, <vscale x 4 x i32> zeroinitializer
+; CHECK: %2 = sext i16 %a to i32
+; CHECK: %.splatinsert1 = insertelement <vscale x 4 x i32> poison, i32 %2, i32 0
----------------
As above, this looks like a redundant test which is not testing anything that is not already covered by the separate signed hi/lo tests.


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsic-opts-unpkhi-unpklo.ll:99-102
+declare <vscale x 4 x i32> @llvm.aarch64.sve.uunpkhi.nxv8i32(<vscale x 8 x i16>)
+declare <vscale x 4 x i32> @llvm.aarch64.sve.uunpklo.nxv8i32(<vscale x 8 x i16>)
+declare <vscale x 4 x i32> @llvm.aarch64.sve.sunpkhi.nxv8i32(<vscale x 8 x i16>)
+declare <vscale x 4 x i32> @llvm.aarch64.sve.sunpklo.nxv8i32(<vscale x 8 x i16>)
----------------
These should end `.nxv4i32` to match the return type.


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

https://reviews.llvm.org/D106929