Page MenuHomePhabricator

[JITLink][RISCV] Initial Support RISCV64 in JITLink
ClosedPublic

Authored by StephenFan on Jul 5 2021, 5:08 AM.

Details

Summary

This patch is the initial support, it implements translation from object file to JIT link graph, and very few relocations were supported. Currently, the test file ELF_pc_indirect.s is passed, the HelloWorld program(compiled with mno-relax flag) can be linked correctly and run on instruction emulator correctly.

In the downstream implementation, I have implemented the GOT, PLT function, and EHFrame and some optimization will be implement soon. I will organize the code in to patches, then gradually send it to upstream.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
lhames added a comment.Jul 8 2021, 2:53 AM

Hi Stephen,

LGTM. This is great stuff -- Thank you for working on this!

llvm/include/llvm/ExecutionEngine/JITLink/ELF_riscv64.h
23–33

Could you please move these edge kinds into their own file (e.g. 'RISCV.h') following the convention established in llvm/include/llvm/ExecutionEngine/JITLink/x86_64.h?

I don't mind if the initial edge set very closely mirrors the ELF relocation set, but the idea is to try to capture the generic fixups that the architecture requires. This simplifies the implementation of new linker backends for the architecture, and allows Plugins to remain as format-agnostic as possible.

Sadly ELF_x86_64 hasn't been moved to the generic kinds in x86_64.h yet, so it's not an ideal example here. The MachO_x86_64 backend is probably a better example.

llvm/lib/ExecutionEngine/JITLink/ELF_riscv64.cpp
24–33

Is this needed? You should be able to use the isDwarfSection method in the base class.

This revision is now accepted and ready to land.Jul 8 2021, 2:53 AM
jrtc27 added a comment.Jul 8 2021, 9:41 AM

As a general point I don't like that this is dubbed a "riscv64" JITLink implementation. Almost all the code is the same between RV32 and RV64, the only difference is whether you have ELFCLASS32 or ELFCLASS64. Renaming ELFJITLinker_riscv64 to ELFJITLinker_riscv, plus renaming ELFLinkGraphBuilder_riscv64 to ELFLinkGraphBuilder_riscv and templating it on the ELFType, should make it "just work" for RV32. At least name the files and public interfaces riscv not riscv64 so adding RV32 support in future won't create avoidable churn.

llvm/include/llvm/ExecutionEngine/JITLink/ELF_riscv64.h
28

I-type but not S-type is a bit odd

llvm/lib/ExecutionEngine/JITLink/CMakeLists.txt
20

Keep sorted

llvm/lib/ExecutionEngine/JITLink/ELF.cpp
70

Sort

85

Sort

llvm/lib/ExecutionEngine/JITLink/ELF_riscv64.cpp
24–33

Can this be moved somewhere so it's shared with x86?

40–42

Dodgy grammar; this is more natural, and also shorter (though you'll need to change the message to R_RISCV_PCREL_LO12_[IS] once you support R_RISCV_PCREL_LO12_S).

45–52

This is strange. You find the first edge that's >= the edge, then walk backwards. That is wrong for two reasons:

  1. You only want to find an R_RISCV_PCREL_HI20 at precisely the right location, not keep walking back until you find one, so should use std::equal_range and iterate over the returned range to look at the relocations _only_ at that specific location.
  2. This is totally the wrong way to do R_RISCV_PCREL_LO12_[IS]; it's _indirect_, so you need to find the relocation by looking at E,getTarget().getAddress() + E.getAddend(), otherwise you'll incorrectly handle things like:
1: auipc a0, %pcrel_hi(foo)
2: auipc a1, %pcrel_hi(bar)
   addi a0, a0, %pcrel_lo(1b)
   addi a1, a1, %pcrel_lo(2b)
1: auipc a0, %pcrel_hi(foo)
   ld t0, %pcrel_lo(1b)(a0)
2: auipc a1, %pcrel_hi(bar)
   ld t1, %pcrel_lo(2b)(a1)
   add t0, t0, t1
   sd t0, %pcrel_lo(1b)(a0)

and

   j 2f
1: addi a0, a0, %pcrel_lo(2f)
   j 3f
2: auipc a0, a0, %pcrel_hi(foo)
   j 1b
3:

all of which are legal, if silly in the case of that last one, though currently LLVM happens to not generate those kinds of things (GCC will generate things like that with -mexplicit-relocs - see https://godbolt.org/z/jPn3M5bM8 - though that's not a default option, and is discouraged, because it's overly aggressive and will do unsound optimisations in certain cases).

88

As discussed in the other review, Lo & 0xFFF is just Value & 0xFFF.

99

Lo needs masking like LO12_I, but also this is then just Value & 0xFFF.

116

Ditto

146

Copy-paste error

152–158

These comments are copied from x86_64 but I don't think they make sense

186

Please follow these clang-tidy suggestions, marking pointers as pointers with auto is the LLVM style

195

Missing return at the front

222

Addends are signed.

llvm/test/ExecutionEngine/JITLink/RISCV/ELF_pc_indirect.s
13

Why is everything 16-byte aligned? And why are you padding with x86 NOPs?

27

Make this lw from a .4byte then the same test could be used for RV32 too

jrtc27 added inline comments.Jul 8 2021, 9:44 AM
llvm/lib/ExecutionEngine/JITLink/ELF_riscv64.cpp
244

OSes other than Linux exist

This revision now requires review to proceed.Jul 8 2021, 10:35 AM

Address @lhames and @jrtc27 's comment

StephenFan marked 5 inline comments as done.Jul 18 2021, 1:33 AM
StephenFan added inline comments.
llvm/include/llvm/ExecutionEngine/JITLink/ELF_riscv64.h
23–33

Currently, the edge set is the mirror of the ELF relocation set. After a while, I will capture the generic fixups and replace the edge set with generic edge set.

jrtc27 added inline comments.Jul 18 2021, 7:15 AM
llvm/include/llvm/ExecutionEngine/JITLink/ELF_riscv64.h
2

Should be ELF_riscv.h

2

Also copy-paste error with that x86-64

llvm/lib/ExecutionEngine/JITLink/ELF.cpp
82

And riscv32

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp
266 ↗(On Diff #359602)

This still hard-codes Linux, as well as that it's 64-bit now the class is just ELFLinkGraphBuilder_riscv.

282–283 ↗(On Diff #359602)
  1. Asserting it's 64-bit _after_ you've cast it is unlikely to end well
  2. Just add the handful of lines to have an ELF32LE case?
289–292 ↗(On Diff #359602)

This can just be link_ELF_riscv, nothing here cares about RV32 vs RV64

StephenFan updated this revision to Diff 359683.EditedJul 19 2021, 12:58 AM
  1. Address @jrtc27 's comment.
  1. Support the RV32, and add test cases to test it
StephenFan added inline comments.Jul 19 2021, 1:04 AM
llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp
266 ↗(On Diff #359602)

To avoid hard-codes, I add an argument to initialize the triple information. Is there any better way to avoid hard-codes ? @jrtc27

lhames added inline comments.Jul 20 2021, 6:07 PM
llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp
266 ↗(On Diff #359602)

Passing an argument in here seems like a reasonable solution to me. You can probably use the triple returned by ObjectFile::makeTriple when you construct the graph builder in createLinkGraphFromELFObject_riscv below.

Address @lhames 's comment.

StephenFan marked 3 inline comments as done.Jul 21 2021, 1:20 AM

@StephenFan -- Thanks for splitting those edge kinds out!

Now that you've moved the edge kinds into their own header the getELFRISCVRelocationKindName function should be renamed/declared as getEdgeKindName in the riscv namespace in riscv.h, and the implementation moved to a new riscv.cpp file (to mirror x86_64::getEdgeKindName). This is a minor request though: I don't mind whether it happens as part of this review, before you commit, or in-tree after it lands. From my point of view this is new functionality, and it'd be a win to land it in-tree sooner rather than later so that other people can start checking it out.

@jrtc27 -- did you have any further fixes that you want before this lands?

Address @lhames 's comments

Replace getELFRISCVRelocationKindName with getEdgeKindName, and moved to riscv.cpp.

jrtc27 added inline comments.Jul 21 2021, 7:55 AM
llvm/lib/ExecutionEngine/JITLink/ELF.cpp
82–83

Sort

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp
57 ↗(On Diff #360446)

Should say PCREL in there otherwise it can be confused with the absolute HI20/LO12 relocations that are more normal (ie don't have this indirection)

90 ↗(On Diff #360446)

This is still using the old inefficient "backwards" way of doing it, just mask Value with 0xFFF and be done with it.

99 ↗(On Diff #360446)

Ditto

118–119 ↗(On Diff #360446)

makes more sense to me, though Expected<const Edge &> would remove one level of indirection for callers so you can just do RelHI20->getTarget() etc.

Address @jrtc27 's comments

StephenFan marked 4 inline comments as done.Jul 21 2021, 8:56 PM
StephenFan marked an inline comment as done.
lhames accepted this revision.Jul 23 2021, 2:29 AM

LGTM.

@StephenFan -- Do you have commit access? If so, please go ahead and commit. If not, are you happy for me to commit this on your behalf?

This revision was not accepted when it landed; it landed in state Needs Review.Jul 23 2021, 8:48 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
thakis added inline comments.
llvm/lib/ExecutionEngine/JITLink/CMakeLists.txt
23

Did you forget to git add this file perchance? :)

LGTM.

@StephenFan -- Do you have commit access? If so, please go ahead and commit. If not, are you happy for me to commit this on your behalf?

Thanks Lang! I have commit this patch.

yaxunl added a subscriber: yaxunl.Jul 23 2021, 8:58 AM

This caused regressions in https://lab.llvm.org/buildbot/#/builders/165/builds/4168

CMake Error at cmake/modules/AddLLVM.cmake:538 (add_library):

Cannot find source file:
  riscv.cpp
Tried extensions .c .C .c++ .cc .cpp .cxx .cu .mpp .m .M .mm .h .hh .h++
.hm .hpp .hxx .in .txx .f .F .for .f77 .f90 .f95 .f03 .ispc

Call Stack (most recent call first):

cmake/modules/AddLLVM.cmake:794 (llvm_add_library)
cmake/modules/AddLLVM.cmake:769 (add_llvm_library)
lib/ExecutionEngine/JITLink/CMakeLists.txt:1 (add_llvm_component_library)
StephenFan marked an inline comment as done.Jul 23 2021, 9:00 AM
StephenFan added inline comments.
llvm/lib/ExecutionEngine/JITLink/CMakeLists.txt
23

I'm sorry. Now, I have made up for my fault. Is it ok now?

thakis added inline comments.Jul 23 2021, 12:35 PM
llvm/lib/ExecutionEngine/JITLink/CMakeLists.txt
23

Yes, bots are happy again. Thanks :)

The test fails in bootstrap builds on all our bots, e.g.:
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8840851478629466864/+/u/package_clang/stdout?format=raw (linux)
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8840851478629466816/+/u/package_clang/stdout?format=raw (windows)

The error is: /b/s/w/ir/cache/builder/src/third_party/llvm-bootstrap/bin/llvm-mc: error: : error: unable to get target for 'riscv64', see --version and --triple.

We don't build in riscv support, so I guess the test needs a REQUIRES line. Oh, actually it's in a RISCV subfolder, so a lit.local.cfg is more appropriate. Added one in 75077f46e7e4d5c89a6d7cd9a8ae7d740df2f4cd, hopefully that helps.

Perhaps revert this patch since it may be good to fix formatting nits in the initial RISC-V JIT patch.

llvm/include/llvm/ExecutionEngine/JITLink/riscv.h
28 ↗(On Diff #361241)

I know this copied from the existing x86_64.h but the documentation style is quite verbose. The blank line and Fixup expression: are redundant.

It is better to use the regular ELF relocation format where symbols like S, A are used to describe operands.

llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp
60 ↗(On Diff #361241)

lld/ELF/RISCV.cpp is not a good example. High before Low is weird.

I may use low&size to represent bits [low, low+size).

134 ↗(On Diff #361241)

previous relocations don't add a blank line.

MaskRay reopened this revision.Jul 25 2021, 2:29 PM

Perhaps revert this patch since it may be good to fix formatting nits in the initial RISC-V JIT patch.

Please don't do that. Formatting nits can easily be fixed in-tree. Getting the code out to people (and testers) is more important.

Perhaps revert this patch since it may be good to fix formatting nits in the initial RISC-V JIT patch.

Please don't do that. Formatting nits can easily be fixed in-tree. Getting the code out to people (and testers) is more important.

The test issues and formatting are not the only problems. The tests insufficient. The absolute and branch relocation types are not tested.

From the description it's clear this is a WIP and not blocking any project.

For initial check-in of some architecture, I think we should set up a higher bar. It looks multiple fixups are not needed for the patch.
It's just cleaner to revert it.

Perhaps revert this patch since it may be good to fix formatting nits in the initial RISC-V JIT patch.

Please don't do that. Formatting nits can easily be fixed in-tree. Getting the code out to people (and testers) is more important.

The test issues and formatting are not the only problems. The tests insufficient. The absolute and branch relocation types are not tested.

From the description it's clear this is a WIP and not blocking any project.

For initial check-in of some architecture, I think we should set up a higher bar. It looks multiple fixups are not needed for the patch.
It's just cleaner to revert it.

As long as the existing tests are passing it's not regressing anything, and represents a monotonic improvement in functionality. More test coverage is better, and a discussion of where testing can/should be improved is good, but at this stage I think this patch is a good fit for in-tree development. I would like it to remain there, and future development to happen in-tree.

If this has landed and isn't going to be reverted, can we please mark this as closed?

StephenFan marked an inline comment as done.Thu, Sep 16, 7:31 PM

If this has landed and isn't going to be reverted, can we please mark this as closed?

There doesn't seem to have the close revision option in the Add Action... bar as usual. Do you have another way to close the revision?

xgupta accepted this revision.Thu, Sep 16, 9:40 PM

It think someone need to accept this revision now to close it.

no luck ):

Although no luck, thanks!

xgupta resigned from this revision.Mon, Sep 20, 5:55 AM

It think someone need to accept this revision now to close it.

Apparently, @jrtc27 needs to withdraw their request for changes first.

jrtc27 removed 1 blocking reviewer(s): jrtc27.Mon, Sep 20, 6:55 AM
This revision is now accepted and ready to land.Mon, Sep 20, 6:55 AM
StephenFan closed this revision.Mon, Sep 20, 8:01 AM