[PATCH] D71773: [AArch64][SVE] Update the definition of AdvSIMD_GatherLoad_VecTorBase_Intrinsic

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12219
+
+  // GLD1_IMM requires that the offset is an immediate in the range 0-31. For
+  // immediates outside that range and non-immediate scalar offsets use GLD1 or
----------------
efriedma wrote:
> sdesmalen wrote:
> > efriedma wrote:
> > > "GLD1_IMM requires that the offset is an immediate in the range 0-31" is not correct, in general; the immediate is multiplied by the element size.
> > This code should not interpret the offset as a byte offset and then scale the offset limit, but rather scale the offset with the element size before swapping Base and Offset.
> > 
> > Perhaps we should rename the intrinsic to `llvm.aarch64.sve.ld1.gather.scalar.index` to clarify the distinction between it being a scaled and unscaled offset (this also gives it the same name as in the ACLE and the architecture spec).
> Does it actually make sense to expose both llvm.aarch64.sve.ld1.gather.scalar.index and  llvm.aarch64.sve.ld1.gather.scalar?  Yes, the ACLE has both "(vector base, scalar index)" and "(vector base, scalar offset in bytes)" intrinsics, but the "(vector base, scalar index)" intrinsics don't map to any single instruction if the index isn't a small constant. You have to emit a separate shift instruction.
You're absolutely right; when I checked the ACLE document, I completely overlooked the intrinsics for scalar byte offsets. I agree it makes little sense to support two intrinsics in that case. Sorry for the confusion!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71773