Page MenuHomePhabricator

[PowerPC] Fix for PC Relative call protocol
ClosedPublic

Authored by stefanp on Jun 3 2020, 5:51 PM.

Details

Summary

The situation where the caller uses a TOC and the callee does not but is marked as clobbers the TOC (st_other=1) was not being compiled correctly if both functions where in the same object file.
The call site where we had callee was missing a nop after the call. This is because it was assumed that since the two functions where in the same DSO they would be sharing a TOC. This is not the case if the callee uses PC Relative because in that case it may clobber the TOC.
This patch makes sure that we add the cnop correctly so that the linker has a place to restore the TOC.

Diff Detail

Event Timeline

stefanp created this revision.Jun 3 2020, 5:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2020, 5:51 PM
This comment was removed by NeHuang.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4756

nit: I think we should update the comment here as we are checking new scenarios below.

4775

The fix makes sense to me.

For the other two scenarios:

  1. Caller using PCRelative addressing, callee using TOC.
  2. Caller using PCRelative addressing, callee using PCRelative addressing.

In both cases, is it possible TOC could be clobbered in the caller (same as the test case below when it call external function externalFunc)? Can we say caller and callee are not sharing the same TOC base?
If so, caller and callee are not share the same TOC but we do not need nop to restore the TOC in the caller.

In this sense, shall we rename this function to callDoesNotRequireTOCRestore since the previous assumption no longer hold for PCRelative addressing?

NeHuang marked an inline comment as done.Jun 12 2020, 12:27 PM
NeHuang added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4702–4703

Seems the assumption "calls share the same TOC base implies call does not require a toc restore" no longer hold when either caller or callee is using PCRelative addressing. Do you think it will be better to rename this function?

nit: In SUMMARY "add the cnop" to "add the nop"

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4775

please ignore this one and found the comment in line 4664

stefanp updated this revision to Diff 271182.Jun 16 2020, 1:20 PM

Fixed the comment and changed the name of the function to have a better description of what it does.

stefanp marked 3 inline comments as done.Jun 18 2020, 7:55 AM
NeHuang accepted this revision as: NeHuang.Jun 18 2020, 7:57 AM

LGTM

This revision is now accepted and ready to land.Jun 18 2020, 7:57 AM
sfertile added inline comments.Jun 18 2020, 10:11 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4763

Could the GlobalValue be a GlobalindirectSymbol instead of a function?

4925

There feels like a disconnect: At one call site we only call the helper function if the subtarget is not using pc-relative calls, but at the other call site we call the helper indiscriminately and rely on logic in the helper to check if the caller is using pc-relative calls. I think its best to keep the helper as callsShareTOCBase and only call it in the case the caller is not using pc-relative calls. I am assuming that we consider a function using pc-relative calls as not maintaining the toc-pointer, and so the answer in that case is trivially no, they do not share a toc base regardless of any other properties.

saghir accepted this revision.Jun 18 2020, 11:58 AM
saghir added a subscriber: saghir.

LGTM

sfertile added inline comments.Jun 18 2020, 1:13 PM
llvm/test/CodeGen/PowerPC/pcrel-local-caller-toc.ll
9

Whats the behavior if we call a function which uses pc-relative instructions to access global data, but has no calls and otherwise preserves the toc-pointer?

63

If this is instead in tail call position, then the test also documents how we can't tail call from a TOC-based caller to a pc-rel based callee, because we need to restore the toc-pointer after the call.

stefanp marked 2 inline comments as done.Jun 22 2020, 9:08 AM
stefanp added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4763

Good point. I'll add another case for the GlobalIndirectSymbol as well.

4925

Actually, we cannot call callsShareTOCBase with PC Relative addressing in either case.
In this case there is a guard just before the call but in getCallOpcode the PC Relative case is handled earlier in the function.

However, I'll add an assert in callsShareTOCBase to make sure that we never reach it when we have PC Relative accesses.

5286

Anything PC Relative shouldn't get past this.

llvm/test/CodeGen/PowerPC/pcrel-local-caller-toc.ll
9

If the function uses PC relative instructions for all access to global data and does not have a call then we mark the function with st_other=0 indicating that it does not clobber the TOC. Unfortunately, we do not have this information early enough to take advantage of it in PPCISelLowering.

63

You are correct. We also cannot tail call because we need to restore the TOC after the call.
This test does not cover that because of the %mul = mul nsw i32 %call, %call after the call but I will add a test to cover that situation.

stefanp updated this revision to Diff 272465.Jun 22 2020, 9:10 AM

Address Review Comments

sfertile added inline comments.Jun 24 2020, 9:37 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4707

If this is only used in the assert then it will causes a warning to be introduced in builds where asserts are disabled.

4763

Hmm, I think we want to try to look through GlobalAliases. Since they must alias a local definition we know the callee, can get it and determine if it uses PCRel or not. If the GlobalIndirectSymbol is an IFunc then the callee is determined at run-time by the dynamic linker calling the resolver function and I don't think we can determine any of the properties of the callee at compile time.

4925

I had missed that thank you. I think the assert you added is a good idea.

sfertile added inline comments.Jun 25 2020, 8:43 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4760

This check was split from the small code model one below since for medium and large code model it was a suffcient condition, while small code model it was only a necessary condition. Now its simply a necessary condition for all code models so we can colapse the 2 checks into a single check with return on false.

If you want some help combining the 2 long comments into one we can do so together in slack if you like.

4784

Do we only check this when targeting large/medium code model because PCRel calls is exclusive to medium code model? If the callee and caller already have different attributes, could they have differing code models as well? The case I'm thinking about would be if callee/caller are in different modules say in a lto or thin lto compile.

stefanp marked 2 inline comments as done.Jun 29 2020, 3:32 AM
stefanp added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4707

Good catch. I'll add an NDEBUG around it.

4784

It's true that PC Rel is only for medium code model. However, as you mentioned, the TOC caller code can be in any model. I'll move this PCRel checking code out of the if statement.

stefanp updated this revision to Diff 274038.Jun 29 2020, 3:38 AM

Reworked the body of callsShareTOCBase to check for PC Relative callees in all cases.
Also, added test cases for ifunc and aliased functions. For the ifunc case I just added test lines to an existing test file.

sfertile added inline comments.Jun 29 2020, 8:50 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4778

I think we can pare down this comment.

The example does a good job at showing why its the callees pre-emptability that matters rather then if the caller/callee are in the same section. I'm not sure that in itself is useful enough to warrant a long comment in the code though. I think the relationship is straightforward enough:

If the callee is preemptable, then the static linker will use a plt-stub which saves the toc to the stack, and needs a nop after the call instruction to convert to a toc-restore.

In this context the given example needing a toc-restore or being un-tail-callable is obvious. I think we should replace the existing comment with something similar to what I used above.

4789

This check is redundant.

llvm/test/CodeGen/PowerPC/func-alias.ll
5

Minor nit: I think having P9 and P10 cases is enough coverage.

llvm/test/CodeGen/PowerPC/ifunc.ll
7

Minor nit: I don't think we need a P9 run. P8 and P10 are enough.

stefanp updated this revision to Diff 274423.Jun 30 2020, 5:01 AM

Fixed up comment and test cases.
Removed condition that was not required.

sfertile accepted this revision as: sfertile.Jun 30 2020, 7:01 AM

LGTM. Thank Stefan.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
4735

real minor nit: 'a valid Alias' --> 'an Alias'

stefanp updated this revision to Diff 274555.Jun 30 2020, 11:33 AM

Fixed final nit.

This revision was automatically updated to reflect the committed changes.