This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add vector mask arithmetic ISel patterns
ClosedPublic

Authored by frasercrmck on Jan 5 2021, 4:06 AM.

Details

Summary

The patterns that want to use 'vnot' use a custom PatFrag. This is
because 'vnot' uses immAllOnesV which implicitly uses BUILD_VECTOR
rather than SPLAT_VECTOR.

Diff Detail

Event Timeline

frasercrmck created this revision.Jan 5 2021, 4:06 AM
frasercrmck requested review of this revision.Jan 5 2021, 4:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2021, 4:06 AM
frasercrmck added inline comments.Jan 5 2021, 4:14 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
41

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.

frasercrmck added inline comments.Jan 5 2021, 6:33 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
41

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
  1. wrap isBuildVectorAllOnes and code that handles SPLAT_VECTOR in yet another function which is (currently) only called by the immAllOnes code.
  1. defer isBuildVectorAllOnes to isConstantSplatVector when it sees ISD::SPLAT_VECTOR.
  1. 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)?

craig.topper added inline comments.Jan 5 2021, 11:58 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
41

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.

frasercrmck added inline comments.Jan 6 2021, 1:44 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
41

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.

craig.topper accepted this revision.Jan 6 2021, 10:42 AM

This patch LGTM. We can work on removing riscv_vnot as a follow up

This revision is now accepted and ready to land.Jan 6 2021, 10:42 AM

rebase onto main

This revision was landed with ongoing or failed builds.Jan 7 2021, 1:49 AM
This revision was automatically updated to reflect the committed changes.