[PATCH] D108987: [RISCV][VP] Custom lower VP_SCATTER and VP_GATHER

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked 4 inline comments as done.
frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4726
+  if (auto *VPGN = dyn_cast<VPGatherSDNode>(Op.getNode())) {
+    MemVT = VPGN->getMemoryVT();
+    MMO = VPGN->getMemOperand();
----------------
craig.topper wrote:
> Can we share MemVT, MMO, Chain, and BasePtr initializing by using the MemSDNode base?
Indeed we can. Thanks. Earlier I was wondering if VP_GATHER & MGATHER and VP_SCATTER & MSCATTER could share an intermediate base class to simplify this code even further or whether that's introducing too deep a hierarchy. Anyway maybe that come later, though I'd like to hear your thoughts.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4739
+    auto *MGN = cast<MaskedGatherSDNode>(Op.getNode());
+    // Else it must be a MGATHER.
+    MemVT = MGN->getMemoryVT();
----------------
craig.topper wrote:
> Should this comment be above the cast?
hah yep.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6412
+    if (N->getOpcode() == ISD::VP_GATHER || N->getOpcode() == ISD::VP_SCATTER) {
+      const auto *VPGSN = cast<VPGatherScatterSDNode>(N);
+      Index = VPGSN->getIndex();
----------------
craig.topper wrote:
> dyn_cast in the above if?
Yeah can do. I'm now using a `dyn_cast`. I originally kept it as an opcode check because I thought it kept it logically a bit closer to the switch statement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108987