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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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:
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? In this sense, shall we rename this function to callDoesNotRequireTOCRestore since the previous assumption no longer hold for PCRelative addressing? |
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 |
Fixed the comment and changed the name of the function to have a better description of what it does.
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. |
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. |
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. 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. |
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. |
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. |
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.
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 | ||
llvm/test/CodeGen/PowerPC/ifunc.ll | ||
7 |
LGTM. Thank Stefan.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
4735 | real minor nit: 'a valid Alias' --> 'an Alias' |
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?