[PATCH] D94078: [RISCV] Add vector mask arithmetic ISel patterns

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td:41
+def riscv_m_vnot : PatFrag<(ops node:$in),
+                           (xor node:$in, (splat_vector (XLenVT 1)))>;
+
----------------
frasercrmck wrote:
> I couldn't find a way of using `SplatPat` here because it complained about using non-named inputs in a PatFrag. Is there a better way? 
> 
> The best way to resolve this would be to fix `OPC_CheckImmAllOnes` but the link with `BUILD_VECTOR` goes quite deep and I can't find the best place to support `SPLAT_VECTOR` without introducing yet another function or moving functionality from `isBuildVectorAllOnes` into `isConstantSplatVector`, which I suspect will introduce a functionality change to many other targets.
So to make some proposals more concrete to provide discussion (everything below also applies to all-zeros):

1. modify the `immAllOnes` SelectionDAGISel code to "inline" handle `SPLAT_VECTOR`

2. wrap `isBuildVectorAllOnes` and code that handles `SPLAT_VECTOR` in yet another function which is (currently) only called by the `immAllOnes` code.

3. defer `isBuildVectorAllOnes` to `isConstantSplatVector` when it sees `ISD::SPLAT_VECTOR`.

4. as above but rename `isBuildVectorAllOnes` to `isConstantSplatAllOnes` to show its new behaviour

Option 4 sounds best to me (because 1 and 2 aren't any good and 3 is half-hearted) but it remains to be seen how big the impact on other targets is. Any thoughts? My initial tests shows that `vnot` works and there's no obvious breakage in other targets.

And since that would affect other targets, would it be a separate patch after this one (replacing `riscv_m_vnot` with `vnot` to show that `immAllOnes` still works)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94078