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

[Craig Topper via Phabricator via llvm-commits]

craig.topper 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:
> 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)?
I agree that option 4 sounds best, but a bulk change like that scares me due to the sheer number of places it is called.

One thought is to rename isBuildVectorAllOnes to isConstantSplatAllOnes, add a bool argument with a default that controls whether it looks at SPLAT_VECTOR. Add back a wrapper isBuildVectorAllOnes that just calls isConstantSplatAllOnes with SPLAT_VECTOR processing disabled. Then we don't have duplicate code and can migrate individual call sites with review and tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94078