Page MenuHomePhabricator

[RISCV][VP] Lower VP ISD nodes to RVV instructions
ClosedPublic

Authored by frasercrmck on Tue, May 4, 4:01 AM.

Details

Summary

This patch supports all of the current set of VP integer binary
intrinsics by lowering them to to RVV instructions. It does so by using
the existing RISCVISD *_VL custom nodes as an intermediate layer. Both
scalable and fixed-length vectors are supported by using this method.

One notable change to the existing vector codegen strategy is that
scalable all-ones and all-zeros mask SPLAT_VECTORs are now lowered to
RISCVISD VMSET_VL and VMCLR_VL nodes to match their fixed-length
BUILD_VECTOR counterparts. This allows them to reuse the existing
"all-ones" VL patterns.

To reduce the size of the phabricator diff, some tests are intentionally
left out and will be added later if the patch is accepted.

Diff Detail

Event Timeline

frasercrmck created this revision.Tue, May 4, 4:01 AM
frasercrmck requested review of this revision.Tue, May 4, 4:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, May 4, 4:01 AM
frasercrmck added inline comments.Tue, May 4, 4:11 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4274

I suspect this could be merged with lowerToScalableOp but I thought I'd leave it separate for now.

craig.topper added inline comments.Tue, May 4, 10:27 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
507

Is this something we should fix in the type legalizer?

craig.topper accepted this revision.Tue, May 4, 10:28 PM

This all looks good to me except the type legalizer question. Which we can probably fix in a follow up. So LGTM

This revision is now accepted and ready to land.Tue, May 4, 10:28 PM

Hi @frasercrmck, tests are using zeroext, is this for simpler code generation before we are able to combine this case and avoid unnecessary sign extensions?

Other than that LGTM too. Thanks!

llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
531–532

Now that you're here: I'm not sure there is a vnot.mm (I fail to find it in the spec) I think this comment should have said vmnot.m, right?

Hi @frasercrmck, tests are using zeroext, is this for simpler code generation before we are able to combine this case and avoid unnecessary sign extensions?

Yeah exactly. I'm using zeroext mostly because it means we can use the same IR and get the same output on both RV32 and RV64 (the zero-extension is not required on RV32 and is eliminated on RV64). Otherwise I think we'd need separated RV32 and RV64 tests. In the real world I don't think we'd get the VL from a function parameter very often so it's a contrived example.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
507

Do you mean that the legalizer would zero-extend the EVL if the operand required promotion, and the VP nodes would have to accommodate an EVL operand that may not be i32? I'd be interested in experimenting with that because we'd get another round of DAG combining on the generic nodes.

I don't have a good feeling for the impact. Maybe @simoll could weigh in.

llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
531–532

Good question. I wonder if it means the vnot.v pseudoinstruction which I see in the 0.10 spec.

  • rebase
  • define getVPLegalizationStrategy now that ExpandVectorPredication is in
This revision was automatically updated to reflect the committed changes.
craig.topper added inline comments.Wed, May 5, 8:26 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
531–532

I think it was supposed to be vmnot.m

frasercrmck marked 2 inline comments as done.Wed, May 5, 8:45 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
531–532

Okay I'll patch that up.

Yeah exactly. I'm using zeroext mostly because it means we can use the same IR and get the same output on both RV32 and RV64 (the zero-extension is not required on RV32 and is eliminated on RV64). Otherwise I think we'd need separated RV32 and RV64 tests. In the real world I don't think we'd get the VL from a function parameter very often so it's a contrived example.

Well I envision a future in which when we vectorize function calls (say via #pragma omp simd) and in some cases they'll be able to receive vl. But given that we'll control them (as in they are not 100% user-provided code) we can always add the zeroext if needed (or whatever makes sense). There is still some time before we can do that though, so not a concern now.