This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Add statistics to show the number of entries in the TOC.
ClosedPublic

Authored by stefanp on Mar 17 2023, 12:42 PM.

Details

Summary

On Power PC some data is stored in the TOC. This pass adds statistics
to show how many entries are emitted to the TOC and what types of
entries those are.

Diff Detail

Event Timeline

stefanp created this revision.Mar 17 2023, 12:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2023, 12:42 PM
stefanp requested review of this revision.Mar 17 2023, 12:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2023, 12:42 PM
stefanp added reviewers: Restricted Project, kamaub.Mar 17 2023, 12:43 PM
stefanp updated this revision to Diff 506179.Mar 17 2023, 12:49 PM

Fixed a couple of spacing issues that I missed when I put the patch up.

stefanp updated this revision to Diff 506655.Mar 20 2023, 11:04 AM

Split the global TOC entries into Local and External Linkage.

amyk added inline comments.Mar 21 2023, 9:52 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
701

Do we not care about the other TLS related target flags (that I suppose are mainly used for Linux)?

llvm/test/CodeGen/PowerPC/ppc-TOC-stats.ll
14
stefanp updated this revision to Diff 509441.Mar 29 2023, 12:30 PM

Updated test case to include another thread local value.
Updated the code to include all of the thread local flags.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
701

Do we not care about the other TLS related target flags (that I suppose are mainly used for Linux)?

This patch is supposed to count TOC entries for both AIX and Linux. On AIX I expect to see all of the TLS entries in the TOC. However, on Linux the entries are in the GOT or they are accessed directly from the TBSS section. You can take a look at the test for this patch to get the idea.

Having said that, I agree that I should add all the rest of the flags here as well. We want to make sure that if there are changes made to AIX in the future they are covered here.

stefanp marked an inline comment as done.Mar 29 2023, 12:31 PM
amyk accepted this revision as: amyk.Mar 31 2023, 11:05 AM

Thanks for addressing my comments. I don't have any further comments but I just had one clarification question.
Other than that, the patch LGTM.

llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
2862

I might be missing something but how do we know that the type will be TOCType_GlobalInternal?
I realized I am not quite sure what the TType means.

This revision is now accepted and ready to land.Mar 31 2023, 11:05 AM
stefanp updated this revision to Diff 510491.Apr 3 2023, 7:16 AM

Based on the question from Amy I realized that I had made an incorrect assumption.
Fixed the TType ouput to check if we have an internal or external global.

stefanp added inline comments.Apr 3 2023, 7:29 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
2862

The TType that we emit here is used for Exception Handling on AIX.
I had originally assumed that exception handling could only be local but after you asked the question I looked into it some more and realized that I was wrong. Therefore, I've fixed this code to check if this is an internal or external symbol.

amyk added inline comments.Apr 3 2023, 8:03 AM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
2862

Thanks Stefan! The change you made LGTM. Thanks for looking into this.

stefanp updated this revision to Diff 510565.Apr 3 2023, 11:15 AM

Did a final rebase to top of main and a clang-format.

This revision was landed with ongoing or failed builds.Apr 3 2023, 11:21 AM
This revision was automatically updated to reflect the committed changes.
nemanjai added inline comments.Apr 3 2023, 2:19 PM
llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp
463–465

Just out of curiosity, why couldn't this have been added to the conditional block on line 476?

llvm/test/CodeGen/PowerPC/ppc-TOC-stats.ll
4

Don't you need something like REQUIRES asserts here?