This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] MC layer support for SiFive VCIX vendor extension.
ClosedPublic

Authored by 4vtomat on Feb 21 2023, 6:40 PM.

Diff Detail

Event Timeline

4vtomat created this revision.Feb 21 2023, 6:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2023, 6:40 PM
4vtomat requested review of this revision.Feb 21 2023, 6:40 PM
4vtomat retitled this revision from [SiFive] Support assembler and dis-assembler for VCIX extension. to Support assembler and dis-assembler for VCIX extension..Feb 21 2023, 6:43 PM
4vtomat added reviewers: craig.topper, kito-cheng.
4vtomat updated this revision to Diff 499348.Feb 21 2023, 6:45 PM

Modify commit message.

Please put SiFive in title since that’s the vendor

Also needs RISCV

craig.topper added inline comments.Feb 21 2023, 8:10 PM
llvm/lib/Target/RISCV/RISCVInstrFormatsV.td
127

Move this to RISCVInstrInfoXsf.td

llvm/lib/Target/RISCV/RISCVInstrInfoXsf.td
102

We need to use DecoderNamespace on these and update RISCVDisassembler to lookup the VCIX table when the extension is enabled. See the other vendor instruction for examples.

Please update docs/ReleaseNotes.rst and docs/RISCVUsage.rst

4vtomat retitled this revision from Support assembler and dis-assembler for VCIX extension. to [SiFive] Support assembler and dis-assembler for VCIX extension..Feb 22 2023, 12:01 AM
4vtomat retitled this revision from [SiFive] Support assembler and dis-assembler for VCIX extension. to [SiFive][RISCV] Support assembler and dis-assembler for VCIX extension..
4vtomat updated this revision to Diff 499404.Feb 22 2023, 1:00 AM

Resolve comments by Craig.

Normally we use the less cumbersome “MC layer” rather than “assembler and dis-assembler” (which also shouldn’t be hyphenated)

craig.topper retitled this revision from [SiFive][RISCV] Support assembler and dis-assembler for VCIX extension. to [RISCV] Support assembler and dis-assembler for SiFive VCIX extension..Feb 22 2023, 12:14 PM
craig.topper retitled this revision from [RISCV] Support assembler and dis-assembler for SiFive VCIX extension. to [RISCV] MC layer support for SiFive VCIX vendor extension..
craig.topper added inline comments.Feb 22 2023, 12:17 PM
llvm/lib/Support/RISCVISAInfo.cpp
123

Please alphabetize above xthead

4vtomat updated this revision to Diff 499703.Feb 22 2023, 6:22 PM

Resolved the comment.

@reames @asb can you help review this?

asb added a comment.Mar 16 2023, 9:00 AM

I haven't looked through the .td description yet, but left some first comments (all pretty easily addressable and minor).

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2721

The usual LLVM style is to use early returns to reduce indentation. see https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

Also, we already have unsigned Opcode = Inst.getOpcode(); above, so you can just use that rather than introducing VCIXOpcode.

llvm/lib/Target/RISCV/RISCVFeatures.td
536

Nit: We're not very consistent, but I think overall it's more common we use title case for the description in parentheses.

llvm/lib/Target/RISCV/RISCVInstrInfo.td
1903

Nit: Shouldn't this be RISCVInstrInfoXSf to match the capitalisation you use in the RISCVUsage addition?

llvm/test/MC/RISCV/rvv/xsfvcp-invalid.s
3

We typically have rv32 CHECK lines as well as rv64

llvm/test/MC/RISCV/rvv/xsfvcp.s
10

We typically have rv32 CHECK lines as well as rv64

4vtomat updated this revision to Diff 506292.Mar 18 2023, 5:52 AM

Resolved Alex's comments.

craig.topper added inline comments.Mar 22 2023, 8:21 PM
llvm/lib/Target/RISCV/RISCVInstrInfoXSf.td
117 ↗(On Diff #506292)

What is the difference between this and CustomSiFiveVCIX?

4vtomat added inline comments.Mar 28 2023, 5:10 PM
llvm/lib/Target/RISCV/RISCVInstrInfoXSf.td
117 ↗(On Diff #506292)

CustomSiFiveVCIF inherits RVInstVCFCustom2 and CustomSiFiveVCIX inherits RVInstVCCustom2.

This revision is now accepted and ready to land.Mar 28 2023, 10:50 PM
asb accepted this revision.Mar 28 2023, 11:09 PM

LGTM too.

This revision was landed with ongoing or failed builds.Apr 9 2023, 8:41 PM
This revision was automatically updated to reflect the committed changes.