This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Implement R_PPC64_REL24_NOTOC local calls. callee has a TOC
ClosedPublic

Authored by NeHuang on Jul 9 2020, 1:15 PM.

Details

Summary

The PC Relative code now allows for calls that are marked with the relocation R_PPC64_REL24_NOTOC. This indicates that the caller does not have a valid TOC pointer in R2 and does not require R2 to be restored after the call. This patch is added to support local calls to callees that require a TOC

Diff Detail

Event Timeline

NeHuang created this revision.Jul 9 2020, 1:15 PM
MaskRay added inline comments.Jul 9 2020, 3:04 PM
lld/ELF/Arch/PPC64.cpp
1054

misaligned

lld/ELF/Target.h
216

Make it imperative, i.e.

// Write a prefixed instruction, which is a 4-byte prefix followed by a 4-byte instruction (regardless of endianness).

"As a result, we need to shift the pieces around on little endian machines." is not needed.

lld/test/ELF/ppc64-pcrel-call-to-toc.s
19

##

23

-NEXT:

NeHuang updated this revision to Diff 277024.Jul 10 2020, 7:12 AM
NeHuang marked 4 inline comments as done.

Addressed comments from MaskRay.

MaskRay added inline comments.Jul 10 2020, 8:41 AM
lld/ELF/Thunks.cpp
1012

What if (s.stOther >> 5) == 1?

lld/test/ELF/ppc64-pcrel-call-to-toc.s
19

The test is created to check that can be omitted. local is not accurate.

  1. When a function without TOC accesses a function using TOC, an r12 setup stub is inserted.
sfertile added inline comments.Jul 13 2020, 9:25 AM
lld/ELF/Thunks.cpp
283

minor nit:

// When the caller does not use TOC and does not preserve R2 makes a local call to a callee that requires a TOC pointer ...

-->

// When a caller that does not maintain a toc-pointer  performs a local call to a callee which requires a toc-pointer ...
852

fatal error if the offset is to large to encode in the instruction.

856

Comment is wrong. destination.getVA() - getThunkTargetSym()->getVA() is the pc-relative offset to the function (func@pcrel), not the offset to the functions plt entry.

862

Good name. We could shorten to '__gep_setup' as I think anyone looking at a PPC64 binary is likely to understand gep is short for global entry point.

sfertile added inline comments.Jul 13 2020, 1:03 PM
lld/ELF/Thunks.cpp
856

Sorry, I missed that the comment also says the instruction is a pld which would be correct if we are loading the symbols address from the plt, but this thunk type is for functions not in the plt. Do you intend to load the address of the function out of a table? or do you meant to calculate the address relative to the program counter?

NeHuang marked 9 inline comments as done.Jul 13 2020, 1:23 PM
NeHuang added inline comments.
lld/ELF/Thunks.cpp
852

Good point. Added fatal error checking if the offset fits in 34 bits.

856

Good catch! Updated it accordingly.

1012

For (s.stOther >> 5) < 2 cases (0, 1), they do not need r12 setup stub but could need a long branch thunk.

NeHuang updated this revision to Diff 277551.EditedJul 13 2020, 1:30 PM
NeHuang marked 2 inline comments as done.

Thanks Sean and MaskRay for the review!

  • Check if the offset computed in PPC64R12SetupStub::writeTo fits in 34 bits and report fatal error if it is too large.
  • Use __gep_setup in the name for r12 set up stub and update test case accordingly.
  • Address nits in the comments.
NeHuang marked 2 inline comments as done.Jul 13 2020, 3:10 PM
NeHuang added inline comments.
lld/ELF/Thunks.cpp
856

No worries. Right, should load the symbol address from the plt instead so that we can use pld to load the callee's global entry point to r12. Will update it accordingly.

Seems fine to me, but of course, wait to hear from Sean/MaskRay.

lld/ELF/Thunks.cpp
855

Would anyone object to naming these in an enum?
Perhaps something like:

enum PPCInstrMasks : uint64_t {
  PLD_NO_DISP = 0x04100000E5800000,
  MTCTR_R12 = 0x7D8903A6,
  ...
};

in llvm/include/llvm/Object/ELF.h?

864

I do not object to the name, but if there is precedent in the GNU linker, we should match it for consistency and familiarity.

NeHuang updated this revision to Diff 277966.EditedJul 14 2020, 2:06 PM
NeHuang marked an inline comment as done.
  • Added a fix to use paddi instead of pld to put the callee global entry point to r12
  • Added enum for PPCInstrMasks in llvm/include/llvm/Object/ELF.h e.g. PADDI_R12_NO_DISP and MTCTR_R12
  • Updated the offset computation for r12 set up stub and test case.
NeHuang marked 4 inline comments as done.Jul 14 2020, 2:17 PM
NeHuang added inline comments.
lld/ELF/Thunks.cpp
855

Thanks for the advice. enum added in ELF.h

864

GNU linker ld uses long_branch.callee as the name, which also used for r2 save stub as mentioned in https://reviews.llvm.org/D82950 . I would go with __gep_steup_ as it represents the thunk type.

NeHuang marked 2 inline comments as done.Jul 14 2020, 2:24 PM
NeHuang updated this revision to Diff 278203.Jul 15 2020, 8:35 AM

Re-based with ToT master branch to merge in other protocol implementations.

I only had a couple of comments.
Overall I think this looks good.

lld/ELF/Arch/PPC64.cpp
113

nit: Why was this moved up and the comment deleted?
I assume it was something that the rebase did...

lld/ELF/Thunks.cpp
873

I feel like there may be an easier way to do this without all of the shifts:

uint64_t paddi = PADDI_R12_NO_DISP |
                 (((offset >> 16) & 0x3ffff) << 32) |
                 (offset & 0xffff);
	​
writePrefixedInstruction(buf + 0, paddi); // paddi r12, 0, func@pcrel, 1

That way you won't have to break the instruction apart and then put it back together.

NeHuang marked 3 inline comments as done.Jul 16 2020, 12:19 PM
NeHuang added inline comments.
lld/ELF/Arch/PPC64.cpp
113

The comment was moved to (line 216) in lld/ELF/Target.h when I added the function declaration there.

lld/ELF/Thunks.cpp
873

Good point! will do.

NeHuang updated this revision to Diff 278599.Jul 16 2020, 2:18 PM
NeHuang marked an inline comment as done.

Thanks Stefan.

  • Modified paddi hex computation with less shifts.
  • Fixed nits for clang-format.
NeHuang marked an inline comment as done.Jul 16 2020, 2:18 PM
MaskRay added inline comments.Jul 16 2020, 2:44 PM
lld/test/ELF/ppc64-pcrel-call-to-toc.s
3
stefanp accepted this revision.Jul 17 2020, 9:30 AM

I only had one question.
Otherwise, LGTM.

However, please wait for approval from sfertile and/or MaskRay before committing.

lld/ELF/Arch/PPC64.cpp
113

Ok, see that the comment was moved.
Why was the function moved up?

This revision is now accepted and ready to land.Jul 17 2020, 9:30 AM
NeHuang marked 2 inline comments as done.Jul 17 2020, 9:57 AM
NeHuang added inline comments.
lld/ELF/Arch/PPC64.cpp
113

I moved it up to group the functions declared in lld/ELF/Target.h (following declaration order) Also separated from the static functions below.

NeHuang marked an inline comment as done.Jul 17 2020, 9:58 AM
NeHuang marked 2 inline comments as done.Jul 17 2020, 10:14 AM
NeHuang added inline comments.
lld/test/ELF/ppc64-pcrel-call-to-toc.s
3

Thanks MaskRay for the input! The new style looks better. Howvever, seems your patch has not landed it yet. : )
I tried applying your changes locally to use extract in this case and confirmed it passed.
To avoid introducing dependency, I prefer keeping the old style in this patch and starting using the new style once your patch committed.

NeHuang marked an inline comment as done.Jul 17 2020, 10:14 AM
MaskRay accepted this revision.Jul 17 2020, 1:34 PM

Thanks!

sfertile accepted this revision.Jul 20 2020, 6:35 AM

LGTM.

This revision was automatically updated to reflect the committed changes.