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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked 3 inline comments as done and an inline comment as not done.
frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4725
+  if (IsVP) {
+    auto *MGN = cast<VPGatherSDNode>(Op.getNode());
+    MemVT = MGN->getMemoryVT();
----------------
craig.topper wrote:
> Couldn't this have been a dyn_cast and avoid the IsVP parameter?
Yeah, I was in two minds about that. Originally I wasn't as big of a fan of `dyn_cast` followed by assert/`cast` - at least `IsVP` allowed us to assert/`cast` on each, requiring the user to pass the right thing. But then that's pretty cumbersome. I've removed the `IsVP`s and I think it looks alright.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6447
       // TODO: Sanitize the scale operand here?
+      // TODO: For VP nodes, should we use VP_SHL here?
       assert(isPowerOf2_32(Scale) && "Expecting power-of-two types");
----------------
rogfer01 wrote:
> I'd say so. Using `ISD::SHL` causes the emission of a `vsetvli` to switch to `vlmax` temporarily. I understand we can do this in a later patch as it seems a low hanging fruit.
> 
> (That said I understand this as part of a deeper problem by itself: `gep`s used to synthesize the vector of pointers fed to `vp.{gather,scatter}` will be already be broken down by DAGBuilder in `vlmax` operations. My wild guess here is that we need something like "demandedvectorelements" but aware of "vlmax" vs "not vlmax").
Yeah you're right: there's quite a lot going on which gets in our way. Even the `SIGN_EXTEND`/`ZERO_EXTEND` above will switch to `vlmax` and I don't think we have a way of solving that. Except, as you suggest, using some clever analysis of users. If all users of an instruction `foo` use a VL of X (or a max of X if you can infer that at compile time) then you're probably pretty safe to reduce the VL of that instruction `foo` down to X. Though I've only considered that for a few seconds so might have missed something :)

I'm not sure "where" that could get done. It's somewhat analogous to the W instructions, isn't it?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:6454
     ISD::MemIndexType NewIndexTy = ISD::UNSIGNED_UNSCALED;
+    if (IsVP) {
+      if (const auto *VPGN = dyn_cast<VPGatherSDNode>(N)) {
----------------
craig.topper wrote:
> Could we just dyn_cast<VPScatterSDNode> and avoiding needing IsVP?
Yeah, here `IsVP` makes less sense. There's already a precedent set by the `dyn_cast` below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108987