[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)))>;
+
----------------
craig.topper wrote:
> 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.
I think your suggestion is a good way of getting this change in relatively atomically, allowing us to complete the work in a later patch.

> can migrate individual call sites with review and tests

Last night I ran `check-all` with where we want to be (`isBuildVectorAllOnes` unconditionally handling splats) and it came back clean. If we were to follow through with that change, are we needing new tests to prove that `SPLAT_VECTOR` works in all existing cases `isBuildVectorAllOnes` is used? That sounds like the ideal situation but a lot of work, and I don't know how best to stress-test the SDAG like that, given that `SPLAT_VECTOR` is still quite the anomaly.

On the other hand, if the existing tests are enough then it should be a relatively quick follow-up. I'm just wary of introducing a "short-term" wrapper that we can't get rid of because we're unable to test all the code paths.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94078