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.



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.

Period at the end of the comment.


Period at the end of the comment.


First letter of variable names should be capitalized.


Can this be

bool isOverlayCall = isa<Function>(GV) && cast<Function>(GV)->hasFnAttribute("overlay-call");

These lines can be shortened slightly to

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

80 columns


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


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


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


Capitalize isOVL


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


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




Move after close curly brace on previous line




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


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

Obviously can't be merged like this


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.


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.


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


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.


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?


Not helpful


This probably warrants just being a separate function

1 ↗(On Diff #371109)

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

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).

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.

19 ↗(On Diff #371109)

Inline it

19 ↗(On Diff #371109)

Inline it

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


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.


Noted, this is definitely unnecessarily restrictive.


I realize these need comments about their usage too

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.