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 ↗(On Diff #499348)

Move this to RISCVInstrInfoXsf.td

llvm/lib/Target/RISCV/RISCVInstrInfoXsf.td
101 ↗(On Diff #499348)

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
133

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
3012

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
644

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
1930

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
118

What is the difference between this and CustomSiFiveVCIX?

4vtomat added inline comments.Mar 28 2023, 5:10 PM
llvm/lib/Target/RISCV/RISCVInstrInfoXSf.td
118

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.