[PATCH] D107957: [LegalizeTypes][VP] Add splitting support for binary VP ops

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked an inline comment as done.
frasercrmck added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:1191
+  assert(VecVT.getVectorElementCount().isKnownEven() &&
+         "Expecting the mask to be an evently-sized vector");
+  unsigned HalfMinNumElts = VecVT.getVectorMinNumElements() / 2;
----------------
craig.topper wrote:
> evently -> evenly
Whoops, thanks.


================
Comment at: llvm/test/CodeGen/RISCV/rvv/vadd-vp.ll:1627
+; FIXME: The first vadd.vi should be able to infer that its AVL is equivalent to VLMAX.
+; FIXME: The second vsetvli is incorrectly preserving the VL on RV64, rather
+; than setting it to 0 as in RV32.
----------------
frasercrmck wrote:
> @craig.topper, do you have any thoughts about this issue? I don't think it's specific to this patch but rather is just exposed by it. We could probably generated a reduced test case on the current `main`.
FWIW this is a problem in `main` but only in the sense that we //could// create something like

```
    %0:gpr = COPY $x0
    dead %1:gpr = PseudoVSETVLI %0, 89, implicit-def $vl, implicit-def $vtype
```

which can then be register-coalesced into `PseudoVSETVLI $x0`. We tend not to generate this sort of thing. It's just that in this example the `COPY $x0` isn't immediately apparent during instruction selection due to the pseudo instructions.

I couldn't find a way of stopping the coalescer do its job here. It's legal from an ISA to do it, but it's not from a semantics point-of-view. It's almost like we need two different `PseudoVSETVLI` instructions: one that takes `x0` and one that doesn't.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107957