This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add RVV insertelt/extractelt scalable-vector patterns
ClosedPublic

Authored by frasercrmck on Jan 13 2021, 10:17 AM.

Details

Summary

Original patch by @rogfer01.

This patch adds support for insertelt and extractelt operations on
scalable vectors.

Special care must be taken on RV32 when dealing with i64 vectors as
there are no straightforward ways to insert a 64-bit element without a
register of that size. To that end, both are custom-lowered to different
sequences.

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

Diff Detail

Event Timeline

frasercrmck created this revision.Jan 13 2021, 10:17 AM
frasercrmck requested review of this revision.Jan 13 2021, 10:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2021, 10:17 AM
  • rebase on main
frasercrmck added inline comments.Jan 21 2021, 3:46 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1751

@craig.topper is it acceptable to use VMV_X_S here, do you think? It relies on the instruction transferring the least-significant XLEN bits even if SEW is larger. However your comment of that node says that the result will never be less than the element size.

craig.topper added inline comments.Jan 21 2021, 11:17 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
774

Maybe we should keep the underscores in the name? Or switch to camel case.

1343

Is this a copy/paste from lowerSPLATVECTOR?

1350

Index isn't signed so this should probably be ZExtOrTrunc?

1751

I think the code in RISCVTargetLoweringComputeNumSignBitsForTargetNode needs to be updated to return 1 if Op.getOperand(0).getScalarValueSizeInBits() is greater than XLen. Other than that it should be fine.

1751

Maybe put XLenVT in a variable. I think it might shorten the EltLo and EltHi lines to not need a line wrap? Or maybe just use MVT::i32 since the code already requires that.

llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
614

This feels like a pretty complicated output sequence. Should we custom lower it?

  • rebase on main
  • address bits of Craig's feedback
frasercrmck marked 4 inline comments as done.Jan 22 2021, 10:12 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
774

Yeah I agree. I was trying to keep the linter happy but I think it's reasonable to override it here.

1343

Hah yeah good catch, thanks.

1350

Makes sense. I had it SExtOrTrunc because it should be correct (?) and it was easier to identify as a sign-extended 32-bit value in the RV32 SPLAT_VECTOR legalization. With ZExtOrTrunc the codegen is currently worse.

Though I think I need to take another look I think this function might be doing splats wrong, and/or creating illegal values. I'll take a look on Monday.

1751

Done. I kept it as XLenVT rather than MVT::i32 but it still works to reduce the bulk.

llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
614

Custom-lowered to a (slideup (vmv (slidedown)))? Or even a (slideup (insertelt (slidedown), val, 0)) ?Might be a good idea given the we need slidedown for RV32 legalization anyway.

frasercrmck retitled this revision from [RISCV][WIP] Add RVV insertelt/extractelt scalable-vector patterns to [RISCV] Add RVV insertelt/extractelt scalable-vector patterns.Jan 22 2021, 10:14 AM
frasercrmck edited the summary of this revision. (Show Details)
frasercrmck marked 3 inline comments as done.
  • rebase on main
  • use custom-lowering and fewer patterns
  • move FromFPR32 helper to anticipate incoming changes
frasercrmck marked 2 inline comments as done.Jan 25 2021, 4:56 AM
frasercrmck added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1350

I've replaced this by directly using SPLAT_VECTOR_I64 to splat the index.

llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td
614

I've updated the patch to always lower insert/extract to slides and an insert/extract of the first element. The patterns are more manageable as a result.

craig.topper accepted this revision.Jan 25 2021, 11:39 AM

LGTM with that comment fix.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2356–2357

While you're here, can you fix substracting->subtracting

This revision is now accepted and ready to land.Jan 25 2021, 11:39 AM
This revision was landed with ongoing or failed builds.Jan 25 2021, 2:10 PM
This revision was automatically updated to reflect the committed changes.