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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/IR/AutoUpgrade.cpp:3883-3884
+    StringRef Name = F->getName();
+    Name = Name.substr(5);
+    if (!Name.startswith("aarch64.sve.tuple")) {
+      DefaultCase();
----------------
Is the name guaranteed to start with `llvm.` ?

If not, I would think it makes more sense to write:

  `if (!Name.startswith("llvm.aarch64.sve.tuple")) {`

Also, should this be "tuple.get" instead of just "tuple" ?


================
Comment at: llvm/lib/IR/AutoUpgrade.cpp:3899
+    Name = Name.substr(5);
+    unsigned N = StringSwitch<Intrinsic::ID>(Name)
+                     .StartsWith("aarch64.sve.tuple.set", 1)
----------------
s/Intrinsic::ID/unsigned/ ?


================
Comment at: llvm/lib/IR/AutoUpgrade.cpp:3910-3919
+    if (N == 1) {
+      unsigned I = dyn_cast<ConstantInt>(CI->getArgOperand(1))->getZExtValue();
+      ScalableVectorType *Ty =
+          dyn_cast<ScalableVectorType>(CI->getArgOperand(2)->getType());
+      Value *NewIdx =
+          ConstantInt::get(Type::getInt64Ty(C), I * Ty->getMinNumElements());
+      NewCall = Builder.CreateCall(
----------------
Rather than this case relying on it being labeled as '1', can we just have:

  if (Name.startswith("llvm.aarch64.sve.tuple.set")) {
    ...
  }

  if (Name.startswith("llvm.aarch64.sve.tuple.create")) {
    // Now parse the number after 'create'
    ...
  }

I think that clarifies the code a bit more.


================
Comment at: llvm/test/Bitcode/upgrade-aarch64-sve-intrinsics.ll:6
+define <vscale x 32 x i8> @ret_svint8x2_t(<vscale x 16 x i8> %unused_z0, <vscale x 16 x i8> %z1, <vscale x 16 x i8> %z2) #0 {
+; CHECK:  %1 = call <vscale x 32 x i8> @llvm.vector.insert.nxv32i8.nxv16i8(<vscale x 32 x i8> poison, <vscale x 16 x i8> %z1, i64 0)
+; CHECK-NEXT:  %tuple = call <vscale x 32 x i8> @llvm.vector.insert.nxv32i8.nxv16i8(<vscale x 32 x i8> %1, <vscale x 16 x i8> %z2, i64 16)
----------------
Can you add a `CHECK-LABEL: @ret_svint8x2_t` to this test (similar for the ones below) ?
(can you just use the update_test_checks.py script for this?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131548