ThinLTO linking runs dataflow processing on collected
function parameters. Then StackSafetyGlobalInfoWrapperPass
in ThinLTO backend will run as usual looking up to external
symbol in the summary if needed.
Depends on D80985.
Paths
| Differential D81242
[StackSafety] Run ThinLTO ClosedPublic Authored by vitalybuka on Jun 5 2020, 1:20 AM.
Details
Summary ThinLTO linking runs dataflow processing on collected Depends on D80985.
Diff Detail
Event TimelineHerald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 5 2020, 1:20 AM Herald added subscribers: llvm-commits, cfe-commits, dexonsmith and 3 others. · View Herald Transcript vitalybuka added a child revision: D81244: [StackSafety] Control paramer access summary from frontend.Jun 5 2020, 2:04 AM
vitalybuka marked 5 inline comments as done. Comment Actions@eugenis review
This revision is now accepted and ready to land.Jun 12 2020, 3:01 PM vitalybuka marked 3 inline comments as done. Comment Actionsaddress comments
Closed by commit rGc1e47b47f884: [StackSafety] Run ThinLTO (authored by vitalybuka). · Explain WhyJun 12 2020, 6:15 PM This revision was automatically updated to reflect the committed changes. Comment Actions I just noticed that generateParamAccessSummary is taking a bit over 5% of the ThinLTO thin link step in an internal build (and will soon be more than 5% as I found another analysis that is hogging compile time that I'm going to work on fixing). This is currently the second hottest analysis in the thin link. Is the stack safety analysis meant to be always on with ThinLTO? I have a theory on what is causing it to incur so much overhead, see below.
Comment Actions
FYI with D84985 now committed, this goes up to >7% of the runtime.
Comment Actions Thanks. Looking.
Comment Actions
During compilation most of the time it should be off. for (auto &GVS : Index) { for (auto &GV : GVS.second.SummaryList) { }} As it paramAccesses suppose to be non-empty for MTE builds for now, so if it's not empty on internal build, then the bug it likely around why it's not empty there. Comment Actions
Will do Comment Actions
Thanks, I was able to reproduce. I'll still check if we can replace guids with ValueInfo to optimize case when StackSafety is active. Comment Actions
Thanks for the fix! I'm surprised simply inserting an entry for every function into a map would be so expensive. This was more expensive than the function importing algorithm, which also does a lot of map insertions (although per GUID not per function summary). Looking at the code, could you improve that by not inserting a map entry in the case where the summary is not live and or not dso local? I.e. the case where the code isn't even looking at the paramAccesses(). Also, note that if any copy in the summary list for a VI is not live, all copies must be dead (the way computeDeadSymbols works guarantees this). So you could skip the SummaryList iteration completely if the first one is not live. I'm also exploiting this fact in my recent fix D84985.
Revision Contents
Diff 270556 clang/test/Driver/memtag_lto.c
llvm/include/llvm/Analysis/StackSafetyAnalysis.h
llvm/lib/Analysis/StackSafetyAnalysis.cpp
llvm/lib/LTO/LTO.cpp
llvm/test/Analysis/StackSafetyAnalysis/ipa-alias.ll
llvm/test/Analysis/StackSafetyAnalysis/ipa.ll
|
Do we have a test for this overflow check?