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

[Graham Hunter via Phabricator via llvm-commits]

huntergr marked 7 inline comments as done.
huntergr added inline comments.


================
Comment at: llvm/lib/CodeGen/InterleavedAccessPass.cpp:477
+    IntrinsicInst *II, SmallVector<Instruction *, 32> &DeadInsts) {
+  if (!II->hasOneUse())
+    return false;
----------------
mgabka wrote:
> could you explain why you are checking it here but not for lowerDeinterleaveIntrinsic?
For deinterleave, I check that the load has a single use so that I can replace both it and the intrinsic -- so the deinterleave intrinsic itself (or a target specific intrinsic which produces the same result) can have multiple uses without problems.

Here, we want to make sure that the store is the only use of the interleave so that both can be replaced.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14765
+  if (EC.isScalable()) {
+    UseScalable = true;
+    return isPowerOf2_32(MinElts) && (MinElts * ElSize) % 128 == 0;
----------------
mgabka wrote:
> 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
It's fine -- if the type is not a legal shuffle type, no transformation will be performed.


================
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
+
----------------
mgabka wrote:
> 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?
This flag allows us to explicitly force the use of SVE ldN/stN instructions for fixed-width vectors. If you look at AArch64TargetLowering::isLegalInterleavedAccessType(), you will see it make a call to Subtarget->forceStreamingCompatibleSVE() as one option for a condition to mark UseScalable = true;

I think the other way to do it would be to set the minimum SVE vector size to 256b, but that only works if the vector types used in the test are >128b. So using the flag means I can force it for all fixed-width tests.


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

https://reviews.llvm.org/D146218