This is an archive of the discontinued LLVM Phabricator instance.

[StripDeadDebugInfo] Drop dead CUs
ClosedPublic

Authored by bader on Mar 21 2022, 11:11 AM.

Details

Summary

In situations when a submodule is extracted from big module (i.e. using
CloneModule) a lot of debug info is copied via metadata nodes. Despite of
the fact that part of that info is not linked to any instruction in extracted
IR file, StripDeadDebugInfo pass doesn't drop them.
Strengthen criteria for debug info that should be kept in a module:

  • Only those compile units are left that referenced by a subprogram debug info

node that is attached to a function definition in the module or to an instruction
in the module that belongs to an inlined function.

  • Compile units are not left explicitly if they have const global expression.

They should be left if that const global is used in a function that is contained
in extracted module.

Signed-off-by: Mikhail Lychkov <mikhail.lychkov@intel.com>

Diff Detail

Event Timeline

mlychkov created this revision.Mar 21 2022, 11:11 AM
mlychkov created this object with edit policy "Administrators".
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 11:11 AM
mlychkov requested review of this revision.Mar 21 2022, 11:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 11:11 AM
mlychkov updated this revision to Diff 417210.Mar 22 2022, 1:47 AM

Reduce code duplication.

mlychkov edited the summary of this revision. (Show Details)Mar 24 2022, 10:14 PM
mlychkov updated this revision to Diff 418136.Mar 24 2022, 11:18 PM
mlychkov changed the edit policy from "Administrators" to "All Users".

Added a check that an instruction is inlined before to try to find inlined CUs.

aprantl added inline comments.Mar 25 2022, 11:15 AM
llvm/lib/Transforms/IPO/StripSymbols.cpp
300

Can you add a doxygen comment explaining what this function does?

301

This is highly subjective, but to me, 32 seems a bit large for a smallptrset. Did you base that number on any performance measurements?

338

This change seems to be unrelated to the above change, and it might be better to discuss it in a separate review. Is this going to drop all constant globals?

374

how about sinking the getInlinedAt() into collectCUsForInlinedFuncs() so we don't check for if(Loc) twice?

ormris removed a subscriber: ormris.May 16 2022, 11:14 AM
bader commandeered this revision.Aug 4 2022, 5:34 AM
bader added a reviewer: mlychkov.
bader updated this revision to Diff 449940.Aug 4 2022, 5:35 AM
bader edited the summary of this revision. (Show Details)

Applied code review suggestions.

bader marked 4 inline comments as done.Aug 4 2022, 5:41 AM

@aprantl, sorry for the delay. I'd like to finish this patch on behalf of @mlychkov.
I've applied all your suggestions. Please, take a look.

llvm/lib/Transforms/IPO/StripSymbols.cpp
301

I reduced 32 -> 8, but I haven't done any performance measurements, so let me know if it sounds reasonable to you.

338

I removed this part of the patch, so let discuss it in a separate review.

bader marked 2 inline comments as done.Aug 4 2022, 5:55 AM
bader added inline comments.Aug 4 2022, 9:19 AM
llvm/lib/Transforms/IPO/StripSymbols.cpp
338

I moved that change to https://reviews.llvm.org/D131179.

bader updated this revision to Diff 450017.Aug 4 2022, 9:27 AM

Apply clang-format. No functional changes.

aprantl accepted this revision.Sep 9 2022, 1:20 PM
This revision is now accepted and ready to land.Sep 9 2022, 1:20 PM
This revision was landed with ongoing or failed builds.Sep 11 2022, 1:34 AM
This revision was automatically updated to reflect the committed changes.
bader added a comment.Sep 11 2022, 1:37 AM

@aprantl, thank you a lot for reviewing this patch. Could you also take a look at the part moved to https://reviews.llvm.org/D131179, please?