This is an archive of the discontinued LLVM Phabricator instance.

[JITLink][ELF][PPC64] Add skeleton ppc64 support and ELF/ppc64 JITLink backend.
ClosedPublic

Authored by lhames on Apr 12 2023, 10:06 PM.

Details

Summary

This patch introduces a skeleton JITLink ppc64 support header and ELF/ppc64
backend. No relocations are supported in this initial version, but given a
program requiring no relocations (e.g. one that just returns a constant value
from main) the new backend is able to construct a LinkGraph from a ppc64 ELF
relocatable object, and the llvm-jitlink tool is able to execute it.

Being minimal, this commit should also serve as a good example of how to
introduce a JITLink backend for a new architecture.

Diff Detail

Event Timeline

lhames created this revision.Apr 12 2023, 10:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2023, 10:06 PM
lhames requested review of this revision.Apr 12 2023, 10:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2023, 10:06 PM

Hi All,

I've added a few of the usual JIT reviewers, and some of you who I've seen make recent contributions to LLVM's PowerPC target library. If there's anyone else who you think should take a look please feel free to add them.

I'm not familiar with PowerPC nomenclature so I'd especially appreciate tips on naming. E.g. should it be "ppc64", "PPC64", "PowerPC64"?

I plan to add at least one basic relocation next. The aim is to make the ELF/ppc64 backend project accessible to the community by getting the JITLink boilerplate out of the way so that any interested committers can focus on implementing the ppc64 specifics.

The skeleton itself is sufficient to get a basic, relocation-free program running:

int main(void) {
  return 0;
}

(As tested on a POWER9 instance that has been generously provided to me by osuosl.org)

The plan sounds very reasonable. PowerPC support is one of the last missing pieces for feature parity with RuntimeDyld. Speaking for the JITLink code, this looks excellent of course.

llvm/test/ExecutionEngine/JITLink/PPC64/ppc64le-no-relocs.s
1 ↗(On Diff #513061)

The test fails for me when I don't build the LLVM PowerPC target. I would have expected it to be unsupported instead. I guess we need a llvm/test/ExecutionEngine/JITLink/PPC64/lit.local.cfg file like this:

if not 'PowerPC' in config.root.targets:
    config.unsupported = True
v.g.vassilev accepted this revision.Apr 13 2023, 4:14 AM
v.g.vassilev added a subscriber: karies.

This looks great!

This revision is now accepted and ready to land.Apr 13 2023, 4:14 AM

Looks good! Does Phabricator only allow one reviewer to accept the revision. I can't seem to find any button that would let me accept the revision.

lhames updated this revision to Diff 513322.Apr 13 2023, 11:45 AM

Add missing lit.cfg.py to llvm/test/ExecutionEngine/JITLink/PPC64 to prevent
ppc64 tests from running when the PowerPC target isn't enabled.

lhames marked an inline comment as done.Apr 13 2023, 11:49 AM
lhames added inline comments.
llvm/test/ExecutionEngine/JITLink/PPC64/ppc64le-no-relocs.s
1 ↗(On Diff #513061)

Thanks for catching this. I've added the missing lit.local.cfg file in the latest version.

sgraenitz accepted this revision.Apr 13 2023, 12:49 PM

Thanks! LGTM

jain98 accepted this revision.Apr 13 2023, 12:51 PM
lhames added a project: Restricted Project.Apr 14 2023, 9:47 AM
lhames marked an inline comment as done.
lhames updated this revision to Diff 513671.Apr 14 2023, 10:36 AM

clang-format

lhames updated this revision to Diff 513928.Apr 15 2023, 10:36 AM

Rename test dir from PPC64 to ppc64.

I'm not sure whether this is the right direction, but it makes the naming
consistent with the rest of the patch (lowercase ppc64 everywhere except
include guards and ELF types/values).

lhames updated this revision to Diff 513935.Apr 15 2023, 11:53 AM

Add a relocation processing loop with no supported relocations.

This will cause the skeleton ELF/ppc64 backend to error out if it encounters
any relocations, rather than silently ignoring them. Having this in the initial
commit helps future developers who may use this skeleton as a template, since
missing relocation failures will self-describe (via the error message) rather
than resulting in a crash or hang.

vchuravy accepted this revision.Apr 15 2023, 1:25 PM
nemanjai accepted this revision.Apr 17 2023, 9:00 AM

This all LGTM and I think the naming is consistent with other targets, so ppc64 makes sense. I have some minor comments that are more likely due to my lack of familiarity with jit-link than anything that should require action on the patch.

llvm/include/llvm/ExecutionEngine/JITLink/ELF_ppc64.h
10

Is it customary for these comments to mention LE/BE separately?
Something like
jit-link functions for ELF/ppc64{le}

22

Should this say ELF/ppc64 since it is for big endian rather than little (le)?

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

I don't know what this does, but I imagine it is not related to PPC support. Since this patch will serve as a guide to future devs, perhaps this should just go in as a separate NFC??? commit?

llvm/lib/ExecutionEngine/JITLink/ELF_ppc64.cpp
47

Is x64 here supposed to be ppc64 or is in just an indication that it is a 64-bit ELF object regardless of architecture?

70

I am just curious what the implications of adding this triple here and whether it would be a problem on little endian PPC64 targets since the triple mentioned here is a big endian triple. Namely, do we need something like

Endianness == support::little ?
  "ppc64le-unknown-linux" :
  "ppc64-unknown-linux"
llvm/test/ExecutionEngine/lit.local.cfg
1 ↗(On Diff #513935)

Just out of curiosity, will this enable all the tests to run on PPC even though a number of them may be failing? Do we need to try this out on PPC and mark the currently failing test cases with XFAIL?

lhames updated this revision to Diff 514346.Apr 17 2023, 11:52 AM

Rebase on top-of-tree, address nemanjai's review comments.

MaskRay accepted this revision.Apr 17 2023, 12:56 PM
MaskRay added a subscriber: MaskRay.
MaskRay added inline comments.
llvm/include/llvm/ExecutionEngine/JITLink/ppc64.h
45

inconsistent "end namespace" marker? :)

Note: with C++17 we can do namespace llvm::jitlink::ppc64

MaskRay added inline comments.Apr 17 2023, 12:57 PM
llvm/lib/ExecutionEngine/JITLink/ELF.cpp
79

The braces are optional.

LLVM style may prefer removing the else, so that the number of lines is even smaller.

lhames marked 4 inline comments as done.Apr 17 2023, 1:11 PM
lhames added inline comments.
llvm/include/llvm/ExecutionEngine/JITLink/ELF_ppc64.h
22

Thanks for catching these!

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

Good point -- the buffer check was just expanded so that we could assume EI_DATA was accessible below, but it's not otherwise related to this patch. I've split it out into d8472ac238d.

llvm/lib/ExecutionEngine/JITLink/ELF_ppc64.cpp
47

Just a typo left in from me copy/pasting the x86-64 backend. :)

70

Yes -- thank you for catching this!

We definitely want to get the architecture in the triple right: ObjectLinkingLayer plugins may use it to make decisions about how they inspect/modify the graph.

llvm/test/ExecutionEngine/lit.local.cfg
1 ↗(On Diff #513935)

This will enable these tests on all platforms where the PowerPC target is built (which I believe is everywhere, except where LLVM_TARGETS_TO_BUILD is explicitly specified to exclude PowerPC).

I've tested this patch on Linux/ppc64le, Linux/x86-64, Darwin/x86-64, and Darwin/arm64 and all tests are passing on those platforms. I think we should probably land this and see what bots (if any) fail, then selectively XFAIL/UNSUPPORT them.

lhames updated this revision to Diff 514408.Apr 17 2023, 2:29 PM
lhames marked 4 inline comments as done and an inline comment as not done.

Use C++17-style for namespaces.

lhames marked an inline comment as done.Apr 17 2023, 2:29 PM
lhames added inline comments.
llvm/include/llvm/ExecutionEngine/JITLink/ppc64.h
45

Good point. I've updated the namespaces in the latest version.

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

I tried this without the braces, but it scans oddly when reading (it looks like incorrectly indented cases). I think it's worth keeping them for grouping here.

Thank you so much for implementing this for PowerPC. Implementing JITLink has been on our radar for a while and this gives us a very welcome head start.

lhames marked an inline comment as done.Apr 18 2023, 3:18 PM

Thank you so much for implementing this for PowerPC. Implementing JITLink has been on our radar for a while and this gives us a very welcome head start.

You're very welcome.

As a next step to landing this I've enabled JIT tests on PowerPC in d771f54107c4. We'll see what (if anything) is failing these days. Once the JIT test suite is passing I think this is ready to land.

Looks like we have a handful of test failures on big-endian machines only when PowerPC tests are enabled (see https://lab.llvm.org/buildbot#builders/93/builds/14488):

Failed Tests (4):
  LLVM :: ExecutionEngine/JITLink/x86-64/MachO-duplicate-local.test
  LLVM :: ExecutionEngine/JITLink/x86-64/MachO_ehframe_bad_fde_cie-ptr_out-of-range.test
  LLVM :: ExecutionEngine/JITLink/x86-64/MachO_ehframe_bad_fde_pc-begin_out-of-range.test
  LLVM :: ExecutionEngine/RuntimeDyld/AArch64/ELF_ARM64_TSTBR14.s

There may be others if the failures are sensitive to memory layout. Still, this looks like a pretty good start.

I'm fine marking the RuntimeDyld tests unsupported on ppc64, but I'd definitely like to get the JITLink ones passing. I've requested a ppc64 big-endian instance from osuosl.org to look into this when I get back from vacation.

It seems that this revision uncovers some existing test failures related to Orc. The failures are only triggered when building the llvm examples with static libraries enabled . Do you guys know who is responsible for fixing these tests?

It seems that this revision uncovers some existing test failures related to Orc. The failures are only triggered when building the llvm examples with static libraries enabled . Do you guys know who is responsible for fixing these tests?

Hi @Dinistro. Do you have a link to the failing tests?

It seems that this revision uncovers some existing test failures related to Orc. The failures are only triggered when building the llvm examples with static libraries enabled . Do you guys know who is responsible for fixing these tests?

Hi @Dinistro. Do you have a link to the failing tests?

Oh -- the failures posted by buildkite! Yep -- I'll be fixing those before this lands.

Oh -- the failures posted by buildkite! Yep -- I'll be fixing those before this lands.

Yes, I was referring to these issues. Thank you for looking into it :)

Quick update on this: osuosl.org is kindly organizing a big-endian PPC64 system for me -- I'm going to use that to debug the big-endian testcase failures that we see when we enable JIT tests for PowerPC. Once those tests are passing we're clear to land this.

It looks like this change has broken MSVC builders: https://lab.llvm.org/buildbot/#/builders/13/builds/35643

llvm-project\llvm\include\llvm/ExecutionEngine/JITLink/ppc64.h(37): error C2220: the following warning is treated as an error
llvm-project\llvm\include\llvm/ExecutionEngine/JITLink/ppc64.h(37): warning C4065: switch statement contains 'default' but no 'case' labels