[PATCH] D120912: [AArch64][SVE] Convert gather/scatter with a stride of 2 to contiguous loads/stores

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

Thanks for making all the changes @kmclaughlin! I think it looks pretty good now. I just had a few more minor comments ...



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16636
+
+    auto EltCount =
+         DAG.getVScale(DL, MVT::i64,
----------------
nit: Looks like this is indented too much here.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16648
+
+    auto MPI = MachinePointerInfo(MGT->getPointerInfo().getAddrSpace());
+    MMO = DAG.getMachineFunction().getMachineMemOperand(
----------------
nit: I think you can also move this above the if statement and then reuse for the store case too, and use MGS->getPointerInfo() instead.


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:364
 
+bool AArch64Subtarget::preferGatherScatter(unsigned Opcode, EVT VecTy, unsigned Stride) const {
+  if (Stride != 2 || !VecTy.isSimple())
----------------
nit: Can you fix the formatting issue? Thanks!


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.h:672
+  // contiguous loads/stores instead.
+  bool preferGatherScatter(unsigned Opcode, EVT VecTy, unsigned Stride) const;
+
----------------
I think it's worth being more explicit here about what `VecTy` actually is because it could be different to the loaded or stored data type. Do you think it's worth renaming this to `MemTy` to be clear? Or you can keep the same variable name, but add a comment saying that is the type for the data in memory. The choice is yours!


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

https://reviews.llvm.org/D120912