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

[mgabka via Phabricator via llvm-commits]

mgabka added inline comments.


================
Comment at: llvm/lib/CodeGen/InterleavedAccessPass.cpp:116
                              SmallVector<Instruction *, 32> &DeadInsts);
 
+  bool lowerDeinterleaveIntrinsic(IntrinsicInst *II,
----------------
perhaps adding a comment to this and the other function below would be a good idea, at least consistent with the other lowerinterleaved* functions


================
Comment at: llvm/lib/CodeGen/InterleavedAccessPass.cpp:477
+    IntrinsicInst *II, SmallVector<Instruction *, 32> &DeadInsts) {
+  if (!II->hasOneUse())
+    return false;
----------------
could you explain why you are checking it here but not for lowerDeinterleaveIntrinsic?


================
Comment at: llvm/lib/CodeGen/InterleavedAccessPass.cpp:521
+    if (auto *II = dyn_cast<IntrinsicInst>(&I)) {
+      if (II->getIntrinsicID() == Intrinsic::experimental_vector_deinterleave2)
+        Changed |= lowerDeinterleaveIntrinsic(II, DeadInsts);
----------------
I think it is worth to add a comment here explaining why we restrict this operation only to VF=2


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14765
+  if (EC.isScalable()) {
+    UseScalable = true;
+    return isPowerOf2_32(MinElts) && (MinElts * ElSize) % 128 == 0;
----------------
is it correct to set it here to true or only if the condition below is true?

from what I can see, before UseScalable was only set to true if this function was returning true


================
Comment at: llvm/test/Transforms/InterleavedAccess/AArch64/fixed-deinterleave-intrinsics.ll:3
+; RUN: opt < %s -interleaved-access -S | FileCheck %s --check-prefix=NEON
+; RUN: opt < %s -interleaved-access -mtriple=aarch64-linux-gnu -mattr=+sve -force-streaming-compatible-sve -S | FileCheck %s --check-prefix=SVE-FIXED
+
----------------
is "-force-streaming-compatible-sve" needed here? I thought that this transformation should happen always for sve, the tests for scalable vectorization have only "target-features"="+sve" added. am I missing something?


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

https://reviews.llvm.org/D146218