Page MenuHomePhabricator

[RISCV][RFC] Add LLVM support for RISC-V overlay system
Needs ReviewPublic

Authored by edward-jones on Sep 7 2021, 9:57 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

For a description of the riscv-overlay system proposal: https://github.com/riscv-software-src/riscv-overlay/

This is the LLVM component of a change to add support for a proposed 'overlay' system for RISC-V to LLVM.

This adds two new function attributes to LLVM, 'overlay-call' and "overlay-data". These functions are used to mark functions and data which must be accessed indirectly through the overlay engine.

For global data marked as 'overlay-data' this change causes a different relocation types to be used when accessing the data. Instead of calculating the address of the data, these relocations calculate the value of the 'token' representing the data in the overlay engine.

For functions marked as 'overlay-call' a similar change occur - a 'token' is used instead of the function address for any references to an 'overlay-call' function, however there is also some additional complexity when handling 'overlay-call':

  • Both calls to an 'overlay-call' function *or* from an 'overlay-call' function must go indirectly through the overlay engine.
  • Calls into the overlay engine use a different calling convention. The entry point to the engine is in x31, and the token value for the target is stored to x30.

An additional change made here is to reserve a number of registers for exclusive used by the overlay engine.

Diff Detail

Event Timeline

edward-jones created this revision.Sep 7 2021, 9:57 AM
edward-jones requested review of this revision.Sep 7 2021, 9:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2021, 9:57 AM
craig.topper added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
627

Period at the end of the comment.

633

Period at the end of the comment.

llvm/lib/LTO/LTO.cpp
1122–1124

First letter of variable names should be capitalized.

1125

Can this be

bool isOverlayCall = isa<Function>(GV) && cast<Function>(GV)->hasFnAttribute("overlay-call");
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
2239

These lines can be shortened slightly to

case RISCV::X1: return STI.hasFeature(RISCV::FeatureReserveX1);
2277

80 columns

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
162

Is this outer if necessary? The inner if and the assert in the inner else should be enough to check the two opcodes.

172

This assert isn't really needed. The getExpr() call on the next line will assert.

174

This assert isn't really needed, the cast<MCSymbolRefExpr> on the next line will assert if it can't cast.

176

Capitalize isOVL

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2896

else should be on the same line as the closing curly brace above

2901

Should this be HiFlags? Why wasn't this caught by the assert that both flags are non-zero in RISCVTargetLowering::getAddr

8372

Capitalize

8384

Move after close curly brace on previous line

8389

Same

8425

Does this condition fit on in 80 columns on one line? It looks like it should

8426

Looks like you could get have one getNode call after the ifs and make the ifs choose only the opcode?

Is there a RISCV document that can be linked in the description?

jrtc27 added inline comments.Sep 7 2021, 10:34 AM
llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def
61

Obviously can't be merged like this

llvm/include/llvm/MC/MCExpr.h
356–357

Why do you need both? Functions get PLTs, objects get data pointers, it should already be implied by the type.

If this is going to be a generic concept then this shouldn't be RISC-V specific.

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

This needs to go through riscv-asm-manual/riscv-toolchain-conventions approval.

Also, when would you ever have this on? It seems like if you're writing assembly you're likely to want to use reserved registers? CodeGen certainly assumes it can, because it knows what they're for, so this would break, say, existing GCC or Clang output. And currently the notion of reserved registers is purely CodeGen and up, not at the MC layer.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
168

This change doesn't cover any of your additions but does cover ones that are already there, so makes no sense

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
125

Even if overlays don't make much sense outside of 32-bit, it would be nicer if the scheme were in theory generalisable to 64-bit, like how R_RISCV_RELATIVE, for example, is for whatever your XLEN is.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
153

Does this need to be expanded so late? It's not like CALL where there's a single relocation for the entire sequence. Can it be expanded earlier like things like LLA?

172

Not helpful

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2817–2825

This probably warrants just being a separate function

llvm/test/CodeGen/RISCV/ovlcall-fncall.ll
1 ↗(On Diff #371109)

Tests need to match the style of existing ones. That includes update_llc_test_checks.py.

49 ↗(On Diff #371109)

This smells a lot like you generated this from C with dodgy code (void foo() and void foo(void) are not the same in C).

124 ↗(On Diff #371109)

optnone makes no sense for these kinds of tests.

Target features belong on the llc call, not per-function (this does matter for the output, as it governs what ends up in the ELF attributes).

llvm/test/MC/RISCV/ovlcall-pseudos.s
10 ↗(On Diff #371109)

This seems redundant, ovlcall should imply overlay

17 ↗(On Diff #371109)

All these implicit register uses really scare me. If you're writing assembly I feel you should just be writing the actual thing, to be honest. This is a lot of hidden magic, and it doesn't seem at all necessary, people are going to be mostly writing in C.

llvm/test/Transforms/Inline/RISCV/riscv-overlaycall.ll
19 ↗(On Diff #371109)

Inline it

llvm/test/Transforms/Inline/RISCV/riscv-overlaycallee.ll
19 ↗(On Diff #371109)

Inline it

Thanks for all of the prompt feedback, I'll incorporate the suggested changes and update the patch

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

Yeah, there's approval needed for the ELF reloc numbers, and the assembler changes too. This patch will be linked from pull requests to those documentation repos.

Good question about warnreservedreg. The motivation here was to help with porting existing RISC-V code which might use registers reserved for the overlay engine, so this option could be selectively disabled if code was being written for the overlay engine itself.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
125

Noted, this is definitely unnecessarily restrictive.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h
108

I realize these need comments about their usage too

llvm/test/MC/RISCV/ovlcall-pseudos.s
17 ↗(On Diff #371109)

Yes, originally there was a motivation to make it simpler to write these calls, but in hindsight I don't think there's a good justification for using a pseudo instruction instead of just being explicit.

I've tried to take into account most of the suggested changes into this update:

  • Tidied up with clang-format
  • Merge 'overlaycall' and 'overlaydata' into a single attribute, just 'overlay'
  • Remove the assembler pseudo instruction and expand the calls into the overlay engine earlier during call lowering. This meant that I could simplify quite a bit. It also means implicit register use/defs aren't hidden by a instruction
  • Fix up the tests
  • Renamed the relocations to be prefixed with RISCV_OVLTOK or RISCV_OVLPLT. I think these names are a bit more descriptive. A OVLTOK reloc encodes a 'token' which only makes sense to the overlay system, whereas a OVLPLT is an address of a thunk which is used to handle indirect functions
  • Use %ovltok_hi/%ovltok_lo/%ovlplt_lo/%ovlplt_hi/@ovlplt/@ovltok as modifiers when writing assembly

I still need to rebase this, and there are some more tests which need to be written. This patch also depends on having a way to encode the reserved register attributes in the attributes in the ELF, but AFAIK there's no patch for that functionality yet.

Some of the previously changes made were undone in order to match the spec as-written (notably I undid the renaming of relocations).

This could do with another review now because I don't expect this patch will change too much now unless the spec also changes.

jrtc27 added a comment.Nov 1 2021, 4:22 AM

Some of the previously changes made were undone in order to match the spec as-written (notably I undid the renaming of relocations).

This could do with another review now because I don't expect this patch will change too much now unless the spec also changes.

Aside from the comment I just made about this attitude on the corresponding Clang patch, standard RISC-V relocations can only be specified by the psABI, so the names absolutely can still be changed if the spec authors propose bad names.

Some of the previously changes made were undone in order to match the spec as-written (notably I undid the renaming of relocations).

This could do with another review now because I don't expect this patch will change too much now unless the spec also changes.

Aside from the comment I just made about this attitude on the corresponding Clang patch, standard RISC-V relocations can only be specified by the psABI, so the names absolutely can still be changed if the spec authors propose bad names.

Okay, I've opened an issue for renaming the relocation in the spec: https://github.com/riscv-software-src/riscv-overlay/issues/29

I'll try and drive the changes to the spec based on feedback first and only then request more feedback here.

edward-jones edited the summary of this revision. (Show Details)Thu, Nov 4, 7:35 AM