[PATCH] D81242: [StackSafety] Run ThinLTO

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added a comment.

In D81242#2190041 <https://reviews.llvm.org/D81242#2190041>, @vitalybuka wrote:

> In D81242#2188467 <https://reviews.llvm.org/D81242#2188467>, @tejohnson wrote:
>
>> In D81242#2188441 <https://reviews.llvm.org/D81242#2188441>, @vitalybuka wrote:
>>
>>> In D81242#2183383 <https://reviews.llvm.org/D81242#2183383>, @tejohnson wrote:
>>>
>>>> Is the stack safety analysis meant to be always on with ThinLTO?
>>>
>>> During compilation most of the time it should be off.
>>> However during linking I assume that most build FS->paramAccesses() is empty, so no hash lookup is expected. So I assume empty looks should be cheap:
>>>
>>>   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.
>>> Can you send me email with internal build details?
>>
>> Will do
>
> Thanks, I was able to reproduce.
> As expected hash table lookup was not reached. However code inserts empty FunctionInfo<FunctionSummary> into std::map Functions. Which does not affect correctness, but totally unnecessary and noticeable on profile.
> https://reviews.llvm.org/rG08cf49658c1d resolves the issue.
>
> I'll still check if we can replace guids with ValueInfo to optimize case when StackSafety is active.

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 <https://reviews.llvm.org/D84985>.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81242/new/

https://reviews.llvm.org/D81242