This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][LLD] Extend R2 save stub to support offsets of more than 26 bits
ClosedPublic

Authored by stefanp on Sep 18 2020, 9:22 AM.

Details

Summary

The R2 save stub will now support offsets up to 64 bits.

There are three cases that will be used.

  1. The offset fits in 26 bits.
b <26 bit offset>
  1. The offset does not fit in 26 bits but fits in 34 bits.
paddi r12, 0, <34 bit offset>, 1
mtctr r12
bctr
  1. The offset does not fit in 34 bits. Since this is an R2 save stub we can use

the TOC in R2. We are not loading the offset but the actual address we want to
branch to.

addis r12, r2, <address in TOC lo>
ld r12 <address in TOC hi>(r12)
mtctr r12
bctr

In case 1) the stub is only 8 bytes while in cases 2) and 3) the stub will be
20 bytes.

Diff Detail

Event Timeline

stefanp created this revision.Sep 18 2020, 9:22 AM
stefanp requested review of this revision.Sep 18 2020, 9:22 AM
stefanp added reviewers: sfertile, NeHuang, Restricted Project.Sep 18 2020, 9:24 AM
MaskRay added inline comments.Sep 18 2020, 9:30 AM
lld/ELF/Thunks.cpp
293

Any reason the alignment is 16?

stefanp added inline comments.Sep 18 2020, 10:52 AM
lld/ELF/Thunks.cpp
293

I've added a prefixed instruction (or the possibility of getting a prefixed instruction) paddi to the Thunk. The issue with prefixed instructions is that they are not allowed to cross a 64 byte boundary. It's a hardware limitation. If I align to 16 bytes I know that the next 16 bytes cannot cross a 64 byte boundary and so my paddi won't cross a 64 byte boundary.

MaskRay accepted this revision.Sep 19 2020, 9:07 PM

Hope another reviewer can verify this patch.

lld/ELF/Thunks.cpp
312

Add const

This revision is now accepted and ready to land.Sep 19 2020, 9:07 PM
sfertile added inline comments.Sep 21 2020, 8:46 AM
lld/ELF/Thunks.cpp
295

Arm thunks avoid oscillating between short and long thunk bodies. The comment says it can prevent the layout from converging. Should we do the same?

932

I know its a bit of a pain, but I think we should rename the local to aviod having a variable name start with an uppercase letter. Maybe 'offsetFromTOC'?

MaskRay added inline comments.Sep 21 2020, 8:51 AM
lld/ELF/Thunks.cpp
295

Ah, yes, we should do it.

NeHuang added inline comments.Sep 21 2020, 1:51 PM
lld/ELF/Thunks.cpp
295

Why do we have isInt<24> instead of isInt<26> here?

stefanp updated this revision to Diff 293543.Sep 22 2020, 11:55 AM

Fixed 24 to 26 bits as the 24 bits was not correct.
Added mayUseShortThunk handling.

MaskRay added inline comments.Sep 22 2020, 1:28 PM
lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
2

This should use PHDRS now to avoid large executables. See D88037

MaskRay requested changes to this revision.Sep 22 2020, 1:28 PM
This revision now requires changes to proceed.Sep 22 2020, 1:28 PM
stefanp updated this revision to Diff 293604.Sep 22 2020, 5:40 PM

Added PHDRS to try to avoid large binary files.

MaskRay added inline comments.Sep 22 2020, 5:51 PM
lld/ELF/Thunks.cpp
925

const

932

const.

lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
2

There are too many \. split-file probably looks better

stefanp updated this revision to Diff 293691.Sep 23 2020, 3:54 AM

Added const in two places.
Added split-file.

MaskRay accepted this revision.Sep 23 2020, 8:49 AM

Looks great! Worth getting @sfertile's sign-off.

This revision is now accepted and ready to land.Sep 23 2020, 8:49 AM
NeHuang accepted this revision as: NeHuang.Sep 23 2020, 2:36 PM

LGTM. Only two nits comment.

lld/test/ELF/ppc64-toc-call-to-pcrel-long-jump.s
51

CHECK-LABEL to CHECK

52

CHECK-LABEL to CHECK-NEXT

sfertile accepted this revision as: sfertile.Sep 24 2020, 6:39 AM

2 minor comments but otherwise LGTM.

lld/ELF/Thunks.cpp
933

Can we change the name to avoid the clang tidy warnings?

NeHuang added inline comments.Sep 24 2020, 7:00 AM
lld/ELF/Thunks.cpp
933

+1

stefanp updated this revision to Diff 294275.Sep 25 2020, 4:38 AM

Fixed last nits.

This revision was landed with ongoing or failed builds.Sep 25 2020, 4:39 AM
This revision was automatically updated to reflect the committed changes.