[PATCH] D115492: [LTO] Ignore unreachable virtual functions in WPD in hybrid LTO

[Mingming Liu via Phabricator via llvm-commits]

luna marked 5 inline comments as done.
luna added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:396
+  }
+  // Identifies a function as unreachable if and only if
+  // 1) All summaries are live.
----------------
tejohnson wrote:
> 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.
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.


================
Comment at: llvm/test/ThinLTO/X86/devirt_hybrid_after_filtering_unreachable.ll:103
+  %0 = bitcast i8* %call to %class.Derived*
+  invoke void @_ZN7DerivedC2Ev(%class.Derived* nonnull align 8 dereferenceable(8) %0)
+          to label %invoke.cont unwind label %lpad
----------------
tejohnson wrote:
> 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?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115492