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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/CodeGen/InterleavedAccessPass.cpp:457
+
+  if (!LI || !LI->hasOneUse())
+    return false;
----------------
Probably best to also check for `isSimple()` to match the behaviour of `lowerInterleavedLoad`.


================
Comment at: llvm/lib/CodeGen/InterleavedAccessPass.cpp:479
+
+  if (!SI)
+    return false;
----------------
Probably best to also check for `isSimple()` to match the behaviour of `lowerInterleavedStore`.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14679-14681
+  VectorType *VTy = dyn_cast<VectorType>(DI->getType()->getContainedType(0));
+  if (!VTy || !VTy->isScalableTy() || !Subtarget->hasSVE())
+    return false;
----------------
I'm happy for incremental bring up but I would like to see fixed length vectors supported sooner rather than later so that we have the option to use the new shuffle intrinsics without losing this feature.  I'm specifically interested in the potential to simplify SVE VLS so that it mirrors SVE VLA 


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14691
+
+  IRBuilder<> Builder(DI);
+  Function *LdNFunc = Intrinsic::getDeclaration(
----------------
This will have the effect of "moving" the load to where the deinterleaving is happening? which has the potential to break the original IR's load/store order.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14719-14721
+  if (VTy->getElementCount().getKnownMinValue() *
+          VTy->getElementType()->getScalarSizeInBits() !=
+      128)
----------------
This is not sufficient because it'll allow `<vscale x 128 x i1>`, `<vscale x 64 x i2>` etc. Perhaps it's better to explicitly check for the types we do support?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14724
+
+  IRBuilder<> Builder(II);
+  Function *StNFunc = Intrinsic::getDeclaration(
----------------
As above, this might create the new "store" in the wrong place.


================
Comment at: llvm/test/Transforms/InterleavedAccess/AArch64/sve-deinterleave-intrinsics.ll:161
+}
+
+;;; Check that (for now) we don't transform when the type won't fit in registers
----------------
Please can you add tests for `ptr` vectors as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146218