[PATCH] D146218: [AArch64][CodeGen] Lower (de)interleave2 intrinsics to ld2/st2

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm accepted this revision.
paulwalker-arm added a comment.
This revision is now accepted and ready to land.

A few recommendations but otherwise looks good.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14748-14750
+  if (!Subtarget->hasNEON() ||
+      (VecTy->isScalableTy() && !Subtarget->hasSVEorSME()))
+    return false;
----------------
Not sure there's a perfect way to write this but I think to be a little more accurate you want something like:
```
if (!VecTy->isScalableTy() && !Subtarget->hasNEON())
  return false;

if (VecTy->isScalableTy() && !Subtarget->hasSVEorSME()))
  return false;
```


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14815
+                                           Type *PtrTy) {
+  static const Intrinsic::ID SVELoads[3] = {Intrinsic::aarch64_sve_ld2_sret,
+                                            Intrinsic::aarch64_sve_ld3_sret,
----------------
Probably worth an `assert(Factor >= 2 && Factor <= 4);`. Same goes for getStructuredStoreFunction.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15209-15210
+    for (unsigned I = 0; I < NumLoads; ++I) {
+      Value *Offset = ConstantInt::get(Type::getInt64Ty(VTy->getContext()),
+                                       I * Factor);
+
----------------
Up to you but given you've got an IRBuilder you could use `Builder.getInt64()` to reduce the line wrapping for all the places where you're using `ConstantInt::get(Type::getInt64Ty(VTy->getContext()),....`.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15262-15265
+  // Need to find the store to obtain the correct insertion point to preserve
+  // load-store ordering.
+  if (!II->hasOneUse())
+    return false;
----------------
Is this code still relevant? It looks like leftovers from before you started to pass `SI` as an operand.


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

https://reviews.llvm.org/D146218