This is an archive of the discontinued LLVM Phabricator instance.

[LTO] Ignore unreachable virtual functions in WPD in hybrid LTO
ClosedPublic

Authored by mingmingl on Dec 9 2021, 11:33 PM.

Details

Summary
  • In FunctionSummary, add a new bit mustBeUnreachable.
  • In ModuleSummaryAnalysis, set this bit to 1 if a function is unreachable.
  • For hybrid LTO, ignore unreachable functions (by reading from index) when finding virtual functions. This helps to de-virtualize. See the test case in llvm/test/ThinLTO/X86/devirt_hybrid_after_filtering_unreachable.ll

Diff Detail

Event Timeline

mingmingl created this revision.Dec 9 2021, 11:33 PM
mingmingl requested review of this revision.Dec 9 2021, 11:33 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 9 2021, 11:33 PM

Revert unit test that shouldn't be affected.

Remove verbose attributes from test case

mingmingl added a subscriber: davidxl.
mingmingl updated this revision to Diff 393921.Dec 13 2021, 9:20 AM

Use GUID computed with exported function name as a fallback to look up index when there isn't an entry for current function GUID.

mingmingl updated this revision to Diff 393927.Dec 13 2021, 9:28 AM

Use type auto for GUID to fix a compile error.

tejohnson added inline comments.Dec 13 2021, 6:43 PM
llvm/lib/Analysis/ModuleSummaryAnalysis.cpp
241

s/unreachbl/unreachable/

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
378

Instead of keeping this in variables, just return false from within the loop immediately whenever we hit one of these conditions (except the non-function summary, see above). Then return true unconditionally after the loop.

396

Actually I think we can safely ignore any summaries that are not not functions - a non-function could have the same GUID in some rare cases, but these are irrelevant to the handling of the functions.

I'm not sure we need all copies to be live, but in general I believe either all will be live or all will be dead. I think we want at least one live function here because the alternative could mean that the prevailing copy was not in IR, in which case we can't make any assumptions. So you could either leave the liveness handling as is, or just ignore any non-live summaries and only return true if we have at least one live summary. In practice both should be the same so it may just be simpler to be conservative if we see any non-live function summaries.

llvm/test/Assembler/thinlto-summary.ll
86

Since this test is checking the round trip of summary through the parser and writer, try adding mustBeUnreachable to the corresponding input summary entries, and try both 0 and 1 values.

llvm/test/ThinLTO/X86/devirt_hybrid_after_filtering_unreachable.ll
104

Is it possible to simplify this test case so that the methods in this file and the other input file don't have all the EH code in them?

mingmingl marked 5 inline comments as done.Dec 14 2021, 8:58 AM
mingmingl added inline comments.
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
396

Done by

  1. ignoring any summaries that are not functions
  2. returning false if any summary is not live
  3. merging "if (!TheFnVI)" and "if (TheFnVI.getSummaryList().empty())"

and added a few lines of comment.

llvm/test/ThinLTO/X86/devirt_hybrid_after_filtering_unreachable.ll
104

I generate the IR by "clang -fvisibility=hidden -fwhole-program-vtables -flto -fsplit-lto-unit -fno-exceptions -S -emit-llvm <file.cc> -o file.ll" now, and the EH symbols and basic blocks are simplified away.

mingmingl updated this revision to Diff 394281.Dec 14 2021, 8:58 AM
mingmingl marked 2 inline comments as done.

Revise based on comments.

tejohnson accepted this revision.Dec 14 2021, 9:26 AM

lgtm after adding a description to test as noted below.

llvm/test/ThinLTO/X86/devirt_hybrid_after_filtering_unreachable.ll
2

Add a brief description at the top as to what this test is for.

This revision is now accepted and ready to land.Dec 14 2021, 9:26 AM
mingmingl updated this revision to Diff 394293.Dec 14 2021, 9:48 AM

Describe what the test case devirt_hybrid_after_filtering_unreachable.ll is for.

lgtm after adding a description to test as noted below.

Done. Thanks for review and feedback!

This revision was landed with ongoing or failed builds.Dec 14 2021, 12:18 PM
This revision was automatically updated to reflect the committed changes.