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

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:241
+// to be unreachable; if it returns false, `F` might still
+// be unreachble but not covered by this helper function.
+static bool mustBeUnreachableFunction(const Function &F) {
----------------
s/unreachbl/unreachable/


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:378
+  }
+  bool AllSummariesAreFunctionSummary = true;
+  bool AllSummariesAreLive = true;
----------------
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.


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


================
Comment at: llvm/test/Assembler/thinlto-summary.ll:86
+; CHECK: ^15 = gv: (guid: 14, summaries: (function: (module: ^1, flags: (linkage: external, visibility: default, notEligibleToImport: 1, live: 1, dsoLocal: 0, canAutoHide: 0), insts: 1, funcFlags: (readNone: 0, readOnly: 0, noRecurse: 0, returnDoesNotAlias: 0, noInline: 1, alwaysInline: 0, noUnwind: 0, mayThrow: 0, hasUnknownCall: 0, mustBeUnreachable: 0))))
+; CHECK: ^16 = gv: (guid: 15, summaries: (function: (module: ^1, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 1, funcFlags: (readNone: 1, readOnly: 0, noRecurse: 1, returnDoesNotAlias: 0, noInline: 0, alwaysInline: 1, noUnwind: 1, mayThrow: 1, hasUnknownCall: 1, mustBeUnreachable: 0))))
+; CHECK: ^17 = gv: (guid: 16, summaries: (function: (module: ^1, flags: (linkage: external, visibility: default, notEligibleToImport: 0, live: 0, dsoLocal: 0, canAutoHide: 0), insts: 1, funcFlags: (readNone: 0, readOnly: 1, noRecurse: 0, returnDoesNotAlias: 1, noInline: 0, alwaysInline: 0, noUnwind: 0, mayThrow: 0, hasUnknownCall: 0, mustBeUnreachable: 0), calls: ((callee: ^15)))))
----------------
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.


================
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
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115492