Page MenuHomePhabricator

[RISCV 5/10] Add bare-bones RISC-V MCTargetDesc
ClosedPublic

Authored by asb on Aug 16 2016, 8:28 AM.

Details

Summary

This is enough to compile and link but doesn't yet do anything particularly useful. Once an ASM parser and printer are added in the next two patches, the whole thing can be usefully tested.

Diff Detail

Repository
rL LLVM

Event Timeline

asb updated this revision to Diff 68188.Aug 16 2016, 8:28 AM
asb retitled this revision from to [RISCV 5/10] Add bare-bones RISC-V MCTargetDesc.
asb updated this object.
asb added reviewers: theraven, jyknight.
asb added a subscriber: llvm-commits.
emaste added a subscriber: emaste.Aug 16 2016, 2:42 PM
reames added a subscriber: reames.Aug 21 2016, 12:18 PM
reames added inline comments.
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
27 ↗(On Diff #68188)

What values can this take? Is this an enum, or a bitmask?

50 ↗(On Diff #68188)

Should this function ever be called? If not, llvm_unreachable.

lib/Target/RISCV/MCTargetDesc/RISCVMCAsmInfo.cpp
21 ↗(On Diff #68188)

OT for this patch, but does the use of a single variable CalleeSaveStackSlotSize imply that we assume all registers which are callee saved have a fixed spill size? This seems strange for architectures which have multiple register classes with different sizes.

jordy.potman.lists added inline comments.
lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
40 ↗(On Diff #68188)

Nit: The LLVM Coding Standards say that comments should use proper capitalization, punctuation, etc.: http://llvm.org/docs/CodingStandards.html#commenting

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
47–48 ↗(On Diff #68188)

Nit: The LLVM Coding Standards say not to duplicate method names in comments: http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

Also applies to the method below.

asb updated this revision to Diff 69350.Aug 26 2016, 5:22 AM
asb marked 4 inline comments as done.

Add llvm_unreachable as suggested by @reames and address nits from @jordy.portman.lists regarding comment formatting.

lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
27 ↗(On Diff #68188)

Ultimately, it should be one of the ELFOSABI_* values defined in llvm/support/ELF.h. Except unlike (for instance) OSType in Triple.h, the enum has no name. The uint8_t type also matches the return type of the static helper MCELFObjectTargetWriter::getOSABI which is used to set its value. I think uint8_t is therefore the best choice here. The value is never inspected in RISCV backend code anyway, it's just passed around.

theraven added inline comments.Aug 26 2016, 6:42 AM
lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
71 ↗(On Diff #69350)

The cast here is odd. It would be cleaner to make Bits a uint32_t and then let the compiler pick the matching instantiation of write<>.

It might be worth a comment saying that this will need changing for variable-length instructions, too (the RISC-V spec currently allows instructions to be up to 320 bits, though I don't know of anyone who is using ones that are longer than 64 bits yet).

asb updated this revision to Diff 69427.Aug 26 2016, 1:01 PM
asb marked an inline comment as done.

Address review comment from @theraven

jyknight accepted this revision.Sep 13 2016, 11:40 AM
jyknight edited edge metadata.
jyknight added inline comments.
lib/Target/RISCV/MCTargetDesc/RISCVMCAsmInfo.cpp
22 ↗(On Diff #69427)

No, it doesn't. We spill according to the register class size.

This value seems only to be used in lib/MC/MCDwarf.cpp, for emitting CFI instructions -- and that use case seems like it's just a factor written to the CIE which all the CFI instructions get multiplied by.

This revision is now accepted and ready to land.Sep 13 2016, 11:40 AM
asb updated this revision to Diff 74224.Oct 11 2016, 3:49 AM
asb edited edge metadata.

Refresh to follow changes made in rL283702, putting TheRISCV32Target and TheRISCV64Target behind accessors.

This revision was automatically updated to reflect the committed changes.