This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add ISel support for RVV vector/scalar forms
ClosedPublic

Authored by frasercrmck on Dec 15 2020, 9:17 AM.

Details

Summary

This patch extends the SDNode ISel support for RVV from only the
vector/vector instructions to include the vector/scalar and
vector/immediate forms.

It uses splat_vector to carry the scalar in each case, except when
XLEN<SEW (RV32 SEW=64) when a custom node SPLAT_VECTOR_I64 is used for
type-legalization and to encode the fact that the value is sign-extended
to SEW. When the scalar is a full 64-bit value we use a sequence to
materialize the constant into the vector register.

The non-intrinsic ISel patterns have also been split into their own
file.

Authored-by: Roger Ferrer Ibanez <rofirrim@gmail.com>
Co-Authored-by: Fraser Cormack <fraser@codeplay.com>

Diff Detail

Event Timeline

frasercrmck created this revision.Dec 15 2020, 9:17 AM
frasercrmck requested review of this revision.Dec 15 2020, 9:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2020, 9:17 AM
craig.topper added inline comments.Dec 15 2020, 10:44 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
353

I'm not sure this should be all scalable types. Typically we only mark the legal types.

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
963

This might not catch all immediates it could. The operand of the splat_vector is going to be XLenVT. When the scalar element type is less than XLenVT, only some of the bits of the splat_vector operand are going to be used so the other bits should be ignored for the purposes of checking if the immediate is simm5. We might get lucky because DAGTypeLegalizer::PromoteIntRes_Constant prefers sign_extend.

craig.topper added inline comments.Dec 15 2020, 10:57 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
353

On second thought, I guess this is ok since we're marking it Legal. Its no different than an operation that defaults to legal. But we are probably going to have to do something different for i1 vectors.

frasercrmck added inline comments.Dec 16 2020, 7:12 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
353

Yeah, I was a bit iffy about it too. I think it's okay for now in that it's currently no worse than BUILD_VECTOR being assumed legal and being used otherwise. But I could also create a list of supported vector MVTs and be specific about which are allowed. We are likely going to need that sort of list in the future anyway.

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
963

That's a good point. I do think PromoteIntRes_Constant is saving us here. Is this something that we should fix without a test case, or something to note for later?

flesh out tests and move them into their own files to avoid conflict-free use
of update_test_checks.py.

re-add dropped changes

  • update lit run lines

rebase on main

kito-cheng added a comment.EditedDec 17 2020, 6:50 AM

Also, it will not handle 64-bit scalars in RV32, but I don't believe that's
actually supported in the spec?

Supported but it might not working as you expected...IIRC it will sign-extend the content of 32 bit reg to 64 bit and then doing a vector-scalar operation.

But maybe you could code gen that by following code seq:

  • push scalar value into stack.
  • load via a stride loads with 0 stride as splat load.
  • vector-vector operation.

Supported but it might not working as you expected...IIRC it will sign-extend the content of 32 bit reg to 64 bit and then doing a vector-scalar operation.

But maybe you could code gen that by following code seq:

  • push scalar value into stack.
  • load via a stride loads with 0 stride as splat load.
  • vector-vector operation.

Oh yes I see that now, thanks Section 11.1:

Vector-scalar operations can have three possible forms, but in all cases take one vector of operands from a vector register group specified by vs2 and a second scalar source operand from one of three alternative sources.
For integer operations, the scalar can be a 5-bit immediate encoded in the rs1 field. The value is sign-extended to SEW bits, unless otherwise specified. . For integer operations, the scalar can be taken from the scalar x register specified by rs1. If XLEN>SEW, the least-significant SEW bits of the x register are used, unless otherwise specified. If XLEN<SEW, the value from the x register is sign-extended to SEW bits.

I was wondering if we could do something like (with a0=lo and a1=hi):

vmv.v.x vX, a1
vsll.vx vX, vX, /*32*/
vor.vx vX, vX, a0

And then do a .vv operation.

We can then optimize for a sign-extended 32-bit value later. What do you think?

I was wondering if we could do something like (with a0=lo and a1=hi):

vmv.v.x vX, a1
vsll.vx vX, vX, /*32*/
vor.vx vX, vX, a0

And then do a .vv operation.

We can then optimize for a sign-extended 32-bit value later. What do you think?

Sounds good idea, but seems like it's correctness issue rather than optimization issue, we can't guarantee the bit 31 isn't 1 unless it's constant literal, the upper-half will become all-1 if it's 1, so...we need 3 more instructions + 1 temp vector reg.

vmv.v.x vX, a1
vsll.vx vX, vX, /*32*/
vmv.v.x vY, a0
vsll.vx vY, vY, /*32*/
vsrl.vx vY, vY, /*32*/
vor.vv vX, vX, vY

and I guess we could borrow FPR for rv32ifdv*:

  • load value to fa0
  • vfmv.v.f vX, fa0

Sounds good idea, but seems like it's correctness issue rather than optimization issue, we can't guarantee the bit 31 isn't 1 unless it's constant literal, the upper-half will become all-1 if it's 1, so...we need 3 more instructions + 1 temp vector reg.

vmv.v.x vX, a1
vsll.vx vX, vX, /*32*/
vmv.v.x vY, a0
vsll.vx vY, vY, /*32*/
vsrl.vx vY, vY, /*32*/
vor.vv vX, vX, vY

and I guess we could borrow FPR for rv32ifdv*:

  • load value to fa0
  • vfmv.v.f vX, fa0

Oh good point, thanks. Interestingly enough riscv-ovpsim didn't clobber the upper bits when I tried running it, but reading the spec I think we agree it should have.

khchen added a subscriber: khchen.Dec 17 2020, 10:40 AM
  • support i64 splats on RV32 using custom nodes and ComplexPats

fix bad patch update

craig.topper added inline comments.Dec 18 2020, 11:40 AM
llvm/lib/Target/RISCV/RISCVISelLowering.h
80–87

Is there a different patch that adds SPLAT_VECTOR_I64_PAIR?

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
878

I don't think you want anything in the square brackets. That autogenerates a pattern for splat_vector/rv32_splat_i64 as the root node for the isel match. I guess your vmv.v.v pattern is getting priority so its not causing a problem?

frasercrmck added inline comments.Dec 18 2020, 11:45 AM
llvm/lib/Target/RISCV/RISCVISelLowering.h
80–87

Sorry, I messed up my first update and it only included my latest commit. You should see the full diff now (if you refresh).

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
878

I'm not sure I follow, sorry. I think I'll need to read up on ComplexPatterns as I've managed to avoid them thus far.

craig.topper added inline comments.Dec 18 2020, 12:00 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
878

The list in square brackets is not used when the ComplexPattern is used as an operand of another node. That's why you have to check for the opcode in the C++.

The bracket list is only used if you write something like this

def : Pat<(SplatPat GPR:$rs)>;

Where the ComplexPattern is the first node of the pattern. You have patterns that where splat_vector, rv32_splat_i64, and rv32_splat_i64 appear as the first node and don't use SplatPat this way.

frasercrmck added inline comments.Dec 18 2020, 12:05 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
878

Right, thanks! I just looked through the TableGen source and I guess the clue's in the name: "root" nodes. I'll get rid of them as you suggest, since I'm not using them.

craig.topper added inline comments.Dec 18 2020, 12:30 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2200

Is it possible to build this sequence from LowerSPLAT_VECTOR using ISD::SHL, ISD::SRL, ISD::OR, RISCVISD::SPLAT_VECTOR_I64, etc? That's probably better than emitting a large sequence in the isel table.

64 bit splat on rv32 code gen sequence is LGTM, but I think that's all I can review :P

  • rebase on main
  • replace SPLAT_VECTOR_I64_PAIR with sequence
  • support or,shl,srl as a consequence
  • remove roots from ComplexPatterns
frasercrmck marked 4 inline comments as done.Dec 21 2020, 4:32 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2200

Agreed, especially since I don't think we'll ever need to do special codegen for SPLAT_VECTOR_I64_PAIR so we might as well expand it early. I've now removed it and expand it in LowerSPLAT_VECTOR.

craig.topper added inline comments.Dec 21 2020, 10:24 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
467

Why is this changed? The name of the class is NoDummyMask.

frasercrmck marked an inline comment as done.

resolve earlier bad rebase with HasDummyMask

frasercrmck marked an inline comment as done.Dec 21 2020, 11:22 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
467

I had a rebase conflict at some point and must have resolved it incorrectly. Thanks for spotting this.

2186

One thing I'm not sure about is whether or not to keep splat_vector and rv32_splat_i64 patterns separate as I've done here or whether to use the ComplexPatterns as root nodes to combine the two. Assuming rv32_splat_i64 is only generated for RV32/i64 and splat_vector isn't created for that case, the result should be the same if a little less clear.

Would it make sense to move the SDNode patterns and their helpers to a different file. They use the pseudos but are otherwise independent. I worry that RISCVInstrInfoVPseudos.td is going to become very large and this might be a logical piece to split out.

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2186

I think it's fine to keep them separate. It definitely won't save anything in the isel table. Tablegen needs to create a different pattern for each root opcode possibility.

frasercrmck marked an inline comment as done.Dec 22 2020, 2:11 AM

Would it make sense to move the SDNode patterns and their helpers to a different file. They use the pseudos but are otherwise independent. I worry that RISCVInstrInfoVPseudos.td is going to become very large and this might be a logical piece to split out.

Could do. I agree the file is going to get unwieldy. However I'd personally find it a bit odd if the RISCVInstrInfoVPseudos.td file contains the pseudos and the intrinsic patterns but not the SDNode patterns. Maybe that's because I view the SDNode support as more "first-class" than intrinsics. I'm assuming you're saying "SDNode" here as opposed to intrinsics, because the intrinsics are technically SDNodes. We just don't have a better terminology.

Would it make sense to move the SDNode patterns and their helpers to a different file. They use the pseudos but are otherwise independent. I worry that RISCVInstrInfoVPseudos.td is going to become very large and this might be a logical piece to split out.

Could do. I agree the file is going to get unwieldy. However I'd personally find it a bit odd if the RISCVInstrInfoVPseudos.td file contains the pseudos and the intrinsic patterns but not the SDNode patterns. Maybe that's because I view the SDNode support as more "first-class" than intrinsics. I'm assuming you're saying "SDNode" here as opposed to intrinsics, because the intrinsics are technically SDNodes. We just don't have a better terminology.

I said SDNode because that seemed to be the naming convention being used for them. I singled them out because right now you’re the only one working on them. We have a lot of intrinsic patches in flight that I didn’t want to interfere with. I don’t know which to view as more first class. The intrinsics will have full instruction coverage. Maybe we put all patterns in separate files. And leave the instructions by themselves. We’re also going to need to support fixed vectors and Simon Moll’s VP intrinsics.

  • rebase on main
  • split patterns into separate file

I said SDNode because that seemed to be the naming convention being used for them. I singled them out because right now you’re the only one working on them. We have a lot of intrinsic patches in flight that I didn’t want to interfere with. I don’t know which to view as more first class. The intrinsics will have full instruction coverage. Maybe we put all patterns in separate files. And leave the instructions by themselves. We’re also going to need to support fixed vectors and Simon Moll’s VP intrinsics.

Yeah I don't think now is the time to move the intrinsic patterns anywhere. I like the sound of eventually having all patterns in separate files though. Take a look at what I've just pushed and see if you think that'll work as a first step.

frasercrmck retitled this revision from [RISCV] Add ISel support for RVV .vx and .vi forms to [RISCV] Add ISel support for RVV vector/scalar forms.Dec 22 2020, 9:30 AM
frasercrmck edited the summary of this revision. (Show Details)
frasercrmck marked 4 inline comments as done.Dec 23 2020, 4:32 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
963

Is this something we can leave for a later patch, now that we've got selectVSplat and friends which can handle it relatively simply?

craig.topper accepted this revision.Dec 23 2020, 11:09 AM

LGTM

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
963

We can leave it for a later patch.

llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
32 ↗(On Diff #313371)

Isn't Complexity the last defaulted argument to ComplexPattern?

This revision is now accepted and ready to land.Dec 23 2020, 11:09 AM
frasercrmck marked an inline comment as done.Dec 23 2020, 11:48 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
32 ↗(On Diff #313371)

Yeah, I'll sort that out before I push. I initially didn't want two empty lists [], [] to get to that argument but I do think it's nicer than a let expression.

This revision was automatically updated to reflect the committed changes.
frasercrmck marked an inline comment as done.