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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
