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

Thu Jan 16 14:34:18 PST 2020

efriedma 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
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71773