[PATCH] D131548: [AArch64]Remove svget/svset/svcreate from llvm

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Could you move the code that adds the combine for `llvm.vector.extract/insert` to a separate patch (along with the beautiful new test), so that we can land that patch before landing D131547 <https://reviews.llvm.org/D131547>?
That also makes this patch simpler, as it will just remove all the svget/svset/svcreate stuff without any other changes.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:2441
+        unsigned VecNumElts = FVTy->getNumElements();
+        unsigned IdxN = cast<ConstantInt>(Idx)->getZExtValue();
+
----------------
nit: if you write the code as:

  auto *FVTy = dyn_cast<FixedVectorType>(Vec->getType());
  auto *DstFVTy = dyn_cast<FixedVectorType>(ReturnType);
  if (FVTy && DstFVTy) {
    ...
  }

you can avoid indenting the whole block (which reduces the size of the diff)


================
Comment at: llvm/test/Transforms/InstCombine/opts-tuples-extract-intrinsic.ll:18-33
+  %1 = call <vscale x 16 x i8> @llvm.vector.extract.nxv16i8.nxv64i8(<vscale x 64 x i8> %a, i64 48)
+  %2 = call <vscale x 16 x i8> @llvm.vector.extract.nxv16i8.nxv64i8(<vscale x 64 x i8> %a, i64 0)
+  %3 = call <vscale x 64 x i8> @llvm.vector.insert.nxv64i8.nxv16i8(<vscale x 64 x i8> %a, <vscale x 16 x i8> %2, i64 48)
+  %4 = call <vscale x 64 x i8> @llvm.vector.insert.nxv64i8.nxv16i8(<vscale x 64 x i8> %3, <vscale x 16 x i8> %1, i64 0)
+  %5 = call <vscale x 16 x i8> @llvm.vector.extract.nxv16i8.nxv64i8(<vscale x 64 x i8> %4, i64 32)
+  %6 = call <vscale x 16 x i8> @llvm.vector.extract.nxv16i8.nxv64i8(<vscale x 64 x i8> %4, i64 16)
+  %7 = call <vscale x 64 x i8> @llvm.vector.insert.nxv64i8.nxv16i8(<vscale x 64 x i8> %4, <vscale x 16 x i8> %6, i64 32)
----------------
CarolineConcatto wrote:
> sdesmalen wrote:
> > I find this test largely incomprehensible :)
> > 
> > Maybe you can split out the two cases in the code into two separate tests:
> > 1. `extract.vector(insert.vector(InsertTuple, InsertValue, Idx), Idx) --> InsertValue`
> > 2. `extract.vector(insert.vector(InsertTuple, InsertValue, Idx1), Idx2) --> extract.vector(InsertTuple, Idx2)`
> > 3. And also add a negative test where the extracted vector-size != inserted vector-size.
> > 
> > For (1):
> >   define <vscale x 16 x i8> @test_extract_insert_same_idx(<vscale x 64 x i8> %v0, <vscale x 16 x i8> %v1) {
> >     ; CHECK-LABEL: @test_extract_insert_same_idx(
> >     ; CHECK-NEXT: ret <vscale x 16 x i8> %v1
> >     %vec.ins = call <vscale x 64 x i8> @llvm.vector.insert.nxv64i8.nxv16i8(<vscale x 64 x i8> %v0, <vscale x 16 x i8> %v1, i64 48)
> >     %vec.ext = call <vscale x 16 x i8> @llvm.vector.extract.nxv16i8.nxv64i8(<vscale x 64 x i8> %vec.ins, i64 48)
> >     ret <vscale x 16 x i8> %vec.ext
> >   }
> > 
> I have changed as you said. 
> But just in case this was the original test we had for svget/svset intrinsics.
> 
That looks great, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131548