[PATCH] D71074: [Aarch64][SVE] Add intrinsics for scatter stores

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11929
+      MaxVT.getSizeInBits().getKnownMinSize())
+    return SDValue();
+
----------------
Is the getKnownMinSize comparison trying to reject illegal types?  Or something else?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11957
+  if (Dst.getValueType().isFloatingPoint())
+    DstNew = DAG.getNode(ISD::BITCAST, DL, HwRetVt, Dst);
+  else
----------------
Does this work correctly for nxv2f32?  It looks like we're missing test coverage, also.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11969
+
+  return DAG.getMergeValues({Store, StoreChain}, DL);
+}
----------------
INTRINSIC_VOID has one result: the chain.  You don't need to MergeValues here.


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:605
 
-  // Scatters using unscaled 32-bit offsets, e.g.
-  //    st1h z0.s, p0, [x0, z0.s, uxtw]
-  // and unpacked:
+  // Scatters using umpacked, unscaled 32-bit offsets, e.g.
   //    st1h z0.d, p0, [x0, z0.d, uxtw]
----------------
"unpacked"


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-scatter-stores-32bit-scaled-offsets.ll:157
+                                                                     double* %base,
+                                                                     <vscale x 2 x i64> %indices)
+  ret void
----------------
Why are the offsets here using `<vscale x 2 x i64>`?  `<vscale x 2 x i32>` seems more natural.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71074