Page MenuHomePhabricator

[PowerPC] Refine the checking for emiting TOC restore nops and Tail-Call eligibility.
ClosedPublic

Authored by sfertile on Jun 15 2017, 11:56 AM.

Details

Summary

In the Small code-model the linker will allocate multiple TOC bases for modules with TOCs that overflow a 16 byte offset. If this happens then all the issues with cross-module calls apply to calls within the same module but with different TOC bases. Since we don't know at compile time where the linker will decide to split the module we can only treat calls within the same section as 'local'. The medium and large code models are expected to provide a sufficiently large TOC to provide all data addressing needs of a module with a single TOC, and so we only need to check if a callee is local to the same module as the caller.

Diff Detail

Repository
rL LLVM

Event Timeline

sfertile created this revision.Jun 15 2017, 11:56 AM
sfertile added a reviewer: inouehrs.
gyiu added inline comments.Jun 15 2017, 1:08 PM
lib/Target/PowerPC/PPCISelLowering.cpp
4254 ↗(On Diff #102695)

Should we just move the code from this function to the existing resideInSameSection() function and rename the function itself? I see in there we're also checking

GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee);
if (!G)
  return true;
sfertile added inline comments.Jun 16 2017, 11:03 AM
lib/Target/PowerPC/PPCISelLowering.cpp
4254 ↗(On Diff #102695)

I think that is a good suggestion. I can't think of any other reason we would want to see if 2 symbols reside in the same section other then if they share a TOC base. I'll update it.

nemanjai added inline comments.Jun 21 2017, 5:24 AM
lib/Target/PowerPC/PPCISelLowering.cpp
4254 ↗(On Diff #102695)

I agree. Both calls to this function are really after the same thing. Although I find this name just slightly misleading. Perhaps something like mayHaveDifferentTOCs() or callRequiresTOCRestoreNop().

test/CodeGen/PowerPC/ppc64-blnop.ll
82 ↗(On Diff #102695)

Well, I think it would be more appropriate to include the call opcode as well - I imagine this one would be bl.

test/CodeGen/PowerPC/ppc64-calls.ll
28 ↗(On Diff #102695)

This comment is misleading. It almost seems to imply that with the large and medium code model, a weak definition can't be overridden in a different module. You should probably say something like "With the large and medium code models, both definitions will have the same TOC since they won't reside in different DSO's."

Or something along those lines.

hfinkel added inline comments.Jun 21 2017, 8:09 AM
lib/Target/PowerPC/PPCISelLowering.cpp
4264 ↗(On Diff #102695)

codemodel should either be "code model" or "CodeModel". My preference is to just write this in English, "small code model", with no extra capitalization.

4265 ↗(On Diff #102695)

This does not explain why this cannot happen for the other code models. The ABI is a bit ambiguous here, saying "The medium code model is expected to provide a sufficiently large TOC to provide all data addressing needs of a module with a single TOC." Expected, however, does not mean "must". We need to supplement here with some additional explanation of what existing linkers actually do (e.g. that neither ld.bfd nor ld.gold will create multiple TOCs for anything other than the small code model, regardless of what might be theoretically allowed).

4266 ↗(On Diff #102695)

I don't see exactly how this reference is relevant. There is some explanation of why you can't tail call to functions in other modules, but if that's not explained in the comments in this file, we should do that directly.

sfertile updated this revision to Diff 103876.Jun 25 2017, 12:51 PM

Addressed a number of the review comments.

sfertile marked 7 inline comments as done.Jun 25 2017, 12:54 PM
sfertile added inline comments.
lib/Target/PowerPC/PPCISelLowering.cpp
4265 ↗(On Diff #102695)

I'm going to send an email to Alan Modra to verify my understanding of the behavior for medium and large code models is correct for both gold and ld. I'll update this comment to reflect that once I get an answer.

sfertile updated this revision to Diff 105469.Jul 6 2017, 10:33 AM

Updated comments describing linker behavior relating to TOC overflow.

sfertile marked 2 inline comments as done.Jul 6 2017, 10:34 AM
inouehrs added inline comments.Jul 6 2017, 11:00 AM
lib/Target/PowerPC/PPCISelLowering.cpp
4252 ↗(On Diff #105469)

In my understanding, ELFv1 uses small code model (please consult with ABI document). If so we need to check ELFv2 ABI.

sfertile added inline comments.Jul 6 2017, 11:50 AM
lib/Target/PowerPC/PPCISelLowering.cpp
4252 ↗(On Diff #105469)

I thought the V1 ABI used small code model for 32 bit and medium code model for 64 bit, but I'll double check.

hfinkel added inline comments.Jul 6 2017, 5:05 PM
lib/Target/PowerPC/PPCISelLowering.cpp
4273 ↗(On Diff #105469)

What happened before binutils 2.24? 2.24 is only about 3.5 years old.

sfertile marked 2 inline comments as done.Jul 6 2017, 6:25 PM
sfertile added inline comments.
lib/Target/PowerPC/PPCISelLowering.cpp
4252 ↗(On Diff #105469)

I dug through the 'V1.9 64 bit ELF ABI supplement ' as well as the older '[32-bit] PowerPC Processor Supplement' and neither mention what code model should be used by default. I think I originally used the gcc docs to infer that ppc64 defaults to medium code model:

IBM RS/6000 and PowerPC Options

-mcmodel=medium

Generate PowerPC64 code for the medium model: The TOC and other static data may be up to a total of 4G in size. This is the default for 64-bit Linux.

and then found 'adjustCodeGenOpts' in PPCMCTargetDesc.cpp which confirmed it.

inouehrs added inline comments.Jul 6 2017, 6:29 PM
lib/Target/PowerPC/PPCISelLowering.cpp
4252 ↗(On Diff #105469)

Sure. Thanks!

sfertile marked an inline comment as done.Jul 6 2017, 8:35 PM
sfertile added inline comments.
lib/Target/PowerPC/PPCISelLowering.cpp
4273 ↗(On Diff #105469)

I don't know. I asked what the behavior of the 2 linkers was if the TOC grows larger then the 32 bit offset, as well as checking that 'module' in the abi text meant entire exe and entire shared-object (rather then just a single object file). I assumed that the behavior change in 2.24 was from generic linker error to specific diagnostic rather then add new toc base --> error. I can follow up though to ensure this is the case.

sfertile updated this revision to Diff 110860.Aug 12 2017, 9:15 PM

Simplified the codemodel check since Default and JitDefault code models were removed.

sfertile marked 2 inline comments as done.Aug 12 2017, 9:31 PM
sfertile added inline comments.
lib/Target/PowerPC/PPCISelLowering.cpp
4273 ↗(On Diff #105469)

Before the change in 2.24 if the offset for *_HI and *_HA relocs didn't fit in 32 bits a 4 instruction sequence would be used to build a 64 bit constant. The change added the 32 bit limit and overflow checking to the *_HI and *_HA relocs and added *_HIGH and *_HIGHA relocs to be used if a 64 bit offset may be needed. So even with the old behavior, its still a single TOC base.

hfinkel accepted this revision.Aug 13 2017, 12:24 AM

LGTM

lib/Target/PowerPC/PPCISelLowering.cpp
4273 ↗(On Diff #105469)

Thanks for checking.

This revision is now accepted and ready to land.Aug 13 2017, 12:24 AM
This revision was automatically updated to reflect the committed changes.
sfertile marked an inline comment as done.