[PATCH] D120912: [AArch64][SVE] Convert gather/scatter with a stride of 2 to contiguous loads/stores
David Sherwood via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 10 07:17:41 PST 2022
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
More information about the llvm-commits
mailing list