This is an archive of the discontinued LLVM Phabricator instance.

[LLD][PowerPC] Add a pc-rel based long branch thunk
ClosedPublic

Authored by NeHuang on Aug 27 2020, 6:17 AM.

Details

Summary

In this patch, a pc-rel based long branch thunk is added for the local call protocol that caller and callee does not use TOC.

Diff Detail

Event Timeline

NeHuang created this revision.Aug 27 2020, 6:17 AM
NeHuang requested review of this revision.Aug 27 2020, 6:17 AM
NeHuang edited reviewers, added: sfertile; removed: Sean.
sfertile added inline comments.Aug 27 2020, 7:18 AM
lld/ELF/Thunks.cpp
375

IIUC If there is a call between a pc-rel based caller and a toc-based callee (so that callee has global and local entry points) then we will have emitted an PPC64R12SetupoStub. Likewise, if its a toc-based caller and pc-rel based callee we use an R2SaveStub. So its only between pc-rel based caller and callee that we can have a pc rel based long branch thunk.

993

The R12Setup stub calculates the address of the callee relative to the program counter.

writePrefixedInstruction(buf + 0, paddi); // paddi r12, 0, func@pcrel, 1
write32(buf + 8, MTCTR_R12);              // mtctr r12
write32(buf + 12, BCTR);                  // bctr

The toc-based long branch thunks have to use a table in the data segment because of limited address-ability. The pc-rel instructions and ABI doesn't have the same limitation, so why use the branch linkage table intead of doing the same thing the R12Setup stubs do?

NeHuang marked 2 inline comments as done.Aug 27 2020, 10:28 AM
NeHuang added inline comments.
lld/ELF/Thunks.cpp
375

Yes. PC-Rel based long branch thunk is needed only between pc-rel based caller and callee.

  • PC-Rel based caller and toc-based callee will use PPC64R12SetupStub
  • Toc-based caller and pc-rel based callee will use PPC64R2SaveStub
993

Good point! Will update the patch without using branch_lt table.

NeHuang updated this revision to Diff 288385.Aug 27 2020, 10:29 AM
NeHuang marked 2 inline comments as done.

Update the patch without using branch_lt table.

nemanjai added inline comments.Aug 27 2020, 10:49 AM
lld/ELF/Thunks.cpp
993

Is it not possible to need a long branch to a destination that isn't available yet? I thought that something like the following would require a long branch to a PLT stub:

#include <stdio.h>
int main(int argc, const char **argv) {
  int a = argc;
  printf("a = %d\n", a);
  asm (".space 134217728");
  printf("a = %d\n", a);
}

But that seems to create two PLT stubs. Is this something that always happens? Namely, is it always the case that the linker will never create a thunk that goes to another thunk? If so, I think a comment explaining that might be useful.

NeHuang added inline comments.Aug 27 2020, 1:31 PM
lld/ELF/Thunks.cpp
993

IIUC we only create a long branch thunk between a caller and callee and we check if there is a valid RelType (R_PPC64_REL14, R_PPC64_REL24, R_PPC64_REL24_NOTOC) in PPC64::needsThunk

NeHuang marked an inline comment as done.Aug 27 2020, 1:38 PM
sfertile added inline comments.Aug 27 2020, 2:15 PM
lld/ELF/Thunks.cpp
993

is it always the case that the linker will never create a thunk that goes to another thunk.

getThunk looks at the symbol referenced by the relocation and sees if an existing compatible thunk can be reused, but if none fit the cirteria it creates a new thunk targeting the symbol rather then creating a thunk that targets another thunk.

Is it not possible to need a long branch to a destination that isn't available yet?

No: all the address sensitive content has been laid out and finalized by the time the thinks writeTo member function is called.

994

I don't believe we have accounted for the addend in the other PPC64 thunks (both toc-based and pc-rel based). If we do take into account the addend then we have to account for that in 'isCompatableWith' as well. I don't believe the compilers emit calls instructions using both symbol and addends (or we likely would have realized this earlier). I suggest to keep it simple we follow the example of the other thinks and omit the addend, or potentially error on a non-zero addend. We can then revisit this in a subsequent patch where we fix all the thunks that will need to account for the addend.

NeHuang marked 2 inline comments as done.Aug 27 2020, 2:34 PM
NeHuang added inline comments.
lld/ELF/Thunks.cpp
994

Agree, will omit the addend in this thunk o be consistent with other PPC64 thunks, we can fix all the thunks to account for the addend in a subsequent patch.

NeHuang updated this revision to Diff 288458.Aug 27 2020, 2:35 PM
NeHuang marked an inline comment as done.
sfertile accepted this revision.Aug 28 2020, 7:04 AM

One minor nit, but otherwise LGTM. If Nemanja would still like the comment he suggested make sure to address that before committing.

lld/ELF/Thunks.cpp
996

Real minor nit: mcmodel=large is a helpful hint, but I think its only relevant if you are using a C/C++ compiler and maybe fortran. It might be confusing if you are linking objects that came from a different source language.

This revision is now accepted and ready to land.Aug 28 2020, 7:04 AM
NeHuang updated this revision to Diff 288610.Aug 28 2020, 7:50 AM

Address Sean's comment for fatal error message.

NeHuang marked an inline comment as done.Aug 28 2020, 7:52 AM
NeHuang added inline comments.
lld/ELF/Thunks.cpp
996

Thanks, message updated.

NeHuang updated this revision to Diff 288618.Aug 28 2020, 8:33 AM
NeHuang marked an inline comment as done.

Updated fatal error message in the LIT test

NeHuang retitled this revision from [LLD][PowerPC] Add pc-rel based long branch thunks to [LLD][PowerPC] Add a pc-rel based long branch thunk.Aug 28 2020, 8:38 AM
NeHuang edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Aug 28 2020, 8:42 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.Nov 4 2021, 2:37 PM
lld/ELF/Thunks.cpp
1143

How is the new PPC64PCRelLongBranchThunk different from PPC64R12SetupStub? Why can't the two be unified?

NeHuang marked an inline comment as done.Nov 11 2021, 8:24 AM
NeHuang added inline comments.
lld/ELF/Thunks.cpp
1143

They are identical. We will create a NFC patch to unify them. Thanks!

MaskRay added inline comments.Nov 26 2021, 12:42 PM
lld/ELF/Thunks.cpp
1143

I created D114656