[PATCH] D148192: [JITLink][ELF][PPC64] Add skeleton ppc64 support and ELF/ppc64 JITLink backend.

[Lang Hames via Phabricator via llvm-commits]

lhames marked 4 inline comments as done.
lhames added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/ELF_ppc64.h:21
+
+/// Create a LinkGraph from an ELF/ppc64le relocatable object.
+///
----------------
nemanjai wrote:
> Should this say `ELF/ppc64` since it is for big endian rather than little (`le`)?
Thanks for catching these!


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF.cpp:61
   StringRef Buffer = ObjectBuffer.getBuffer();
-  if (Buffer.size() < ELF::EI_MAG3 + 1)
+  if (Buffer.size() < ELF::EI_NIDENT)
     return make_error<JITLinkError>("Truncated ELF buffer");
----------------
nemanjai wrote:
> 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?
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.



================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_ppc64.cpp:46
+        return make_error<StringError>(
+            "No SHT_REL in valid x64 ELF object files",
+            inconvertibleErrorCode());
----------------
nemanjai wrote:
> Is `x64` here supposed to be `ppc64` or is in just an indication that it is a 64-bit ELF object regardless of architecture?
Just a typo left in from me copy/pasting the x86-64 backend. :)


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_ppc64.cpp:69
+                            const object::ELFFile<ELFT> &Obj)
+      : ELFLinkGraphBuilder<ELFT>(Obj, Triple("ppc64-unknown-linux"), FileName,
+                                  ppc64::getEdgeKindName) {}
----------------
nemanjai wrote:
> 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"
> ```
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.


================
Comment at: llvm/test/ExecutionEngine/lit.local.cfg:1
-if config.root.native_target in ['Sparc', 'PowerPC', 'SystemZ', 'Hexagon', 'RISCV']:
+if config.root.native_target in ['Sparc', 'SystemZ', 'Hexagon', 'RISCV']:
     config.unsupported = True
----------------
nemanjai wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148192/new/

https://reviews.llvm.org/D148192