[PATCH] D103939: [SVE][LSR] Teach LSR to enable simple scaled-index addressing mode generation for SVE.

[Huihui Zhang via Phabricator via llvm-commits]

huihuiz added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:11810
+    return AM.HasBaseReg && !AM.BaseOffs &&
+           (AM.Scale == 0 || AM.Scale == 1 ||
+            (AM.Scale > 0 && (uint64_t)AM.Scale == VecElemNumBytes));
----------------
dmgreen wrote:
> What does a AM.Scale == 1 relate to? Using a ld1b?
For ld1b cases, AM.Scale could be 1 when trying to match [r+r] with a base register.
But such case is covered by "(uint64_t)AM.Scale == VecElemNumBytes", since VecElemNumBytes is also 1.

I simplify this checking into "(AM.Scale == 0 || (uint64_t)AM.Scale == VecElemNumBytes)". 
Let me know if there is any case I missed ?


================
Comment at: llvm/test/CodeGen/AArch64/sve-lsr-scaled-index-addressing-mode.ll:41
+  %out.ptrcast = bitcast i16* %ptr.out to <vscale x 8 x i16>*
+  %val = load volatile <vscale x 8 x i16>, <vscale x 8 x i16>* %in.ptrcast, align 16
+  %addp_vec = add <vscale x 8 x i16> %val, %p_vec.splat
----------------
dmgreen wrote:
> Why is this load volatile?
Removed, not needed.


================
Comment at: llvm/test/CodeGen/AArch64/sve-lsr-scaled-index-addressing-mode.ll:57
+
+declare <vscale x 8 x i16> @llvm.masked.load.nxv8i16.p0nxv8i16(<vscale x 8 x i16>*, i32 immarg, <vscale x 8 x i1>, <vscale x 8 x i16>)
+
----------------
dmgreen wrote:
> This test isn't using masked loads/stores, but it would be good to make sure they work sensibly.
Thanks for catching it! masked load/store case added, work as expected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103939