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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td | ||
---|---|---|
41 | So to make some proposals more concrete to provide discussion (everything below also applies to all-zeros):
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)? |
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. |
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.
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. |
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.