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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.

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.



================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/ELF_ppc64.h:9
+//
+// jit-link functions for ELF/ppc64.
+//
----------------
Is it customary for these comments to mention LE/BE separately?
Something like
`jit-link functions for ELF/ppc64{le}`


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


================
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");
----------------
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?


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


================
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) {}
----------------
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"
```


================
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
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148192