[PATCH] D155672: [JITLink][PowerPC] Correct handling of R_PPC64_REL24_NOTOC

[Kai Luo via Phabricator via llvm-commits]

lkail added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/ppc64.h:54-57
 extern const char PointerJumpStubContent_big[20];
 extern const char PointerJumpStubContent_little[20];
+extern const char PointerJumpStubNoTOCContent_big[32];
+extern const char PointerJumpStubNoTOCContent_little[32];
----------------
lhames wrote:
> You could further unify some of the code paths using a pair of template variables:
> 
> ```
> template <support::endianness Endianness>
> ArrayRef<char> PtrJumpStubContent;
> 
> template<>
> ArrayRef<char> PtrJumpStubContent<support::endianness::big> =
>     PtrJumpStubContent_big;
> template<>
> ArrayRef<char> PtrJumpStubContent<support::endianness::litttle> =
>     PtrJumpStubContent_little;
> 
> template <support::endianness Endianness>
> ArrayRef<char> PtrJumpStubNotTOCContent;
> 
> template<>
> ArrayRef<char> PtrJumpStubNoTOCContent<support::endianness::big> =
>     PtrJumpStubNoTOCContent_big;
> template<>
> ArrayRef<char> PtrJumpStubNoTOCContent<support::endianness::litttle> =
>     PtrJumpStubNoTOCContent_little;
> ```
> 
> `pickStub`'s cases then look like:
> ```
> template <support::endianness Endianness>
> inline PLTCallStubInfo pickStub(PLTCallStubKind StubKind) {
>   switch (StubKind) {
>   case LongBranch: {
>     auto Content = PointerJumpStubContent<Endianness>;
>     // Skip save r2.
>     Content = Content.slice(4);
>     return PLTCallStubInfo{
>         Content,
>         {{TOCDelta16HA, 0, 0}, {TOCDelta16LO, 4, 0}},
>     };
>   }
>   case LongBranchSaveR2: {
>     auto Content = PointerJumpStub<Endianness>;
>     return PLTCallStubInfo{
>         Content,
>         {{TOCDelta16HA, 4, 0}, {TOCDelta16LO, 8, 0}},
>     };
>   }
>   case LongBranchNoTOC: {
>     auto Content = PointerJumpStubNoTOCContent<Endianness>;
>     return PLTCallStubInfo{
>         Content,
>         {{Delta16HA, 16, 8}, {Delta16LO, 20, 12}},
>     };
>   }
>   }
> }
> ...
> ```
> 
> If other clients are likely to use the stub content arrays this may be worth it. If everyone is going to use `pickStub` then I think it's fine either way. 
> 
Looks great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155672