[PATCH] D116056: [LTO][WPD] Ignore unreachable function by analyzing IR.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

lgtm



================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:2110
+  // FWIW, the writer of `ExportSummary` guarantees that function declarations
+  // won't have an entry in `ExportSummary`;
+  // see llvm/lib/Analysis/ModuleSummaryAnalysis.cpp#L785 at commit cf9e61a for
----------------
I don't think this comment is really relevant here. Note you are coming into this part of the handling specifically when we *do* have a function declaration in this module, so it is a little confusing to explain why we don't create a summary for them. The reason why we may have a summary for what is a function decl in this combined regular LTO module is that we have the combined summaries of all of the ThinLTO modules, and the def is in one of those. So I would just keep the first sentence of this comment block ("Learn unreachability from ...").


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1056
+
     if (ExportSummary && (mustBeUnreachableFunction(
                              lookUpFunctionValueInfo(Fn, ExportSummary)))) {
----------------
luna wrote:
> tejohnson wrote:
> > Since you are adding the IR check above, thinking that to optimize for compile time this lookup should be ignored if we have the def of Fn available (avoids an extra hash table lookup and analysis).
> Done by "first analyze function def in IR if that's present, and use export summary as a fallback".
> 
> I took the liberty to add a comment like [1]; I'm wondering what's the idiomatic way to provide pointer a line of code w/ commit for reference?
> 
> [1] 
> ```
> // FWIW, the writer of `ExportSummary` guarantees that function declarations
>   // won't have an entry in `ExportSummary`;
>   // see llvm/lib/Analysis/ModuleSummaryAnalysis.cpp#L785 at commit cf9e61a for
>   // a reference.
> ```
That's a good question on code reference. Usually I see references to the file or function granularity, not this specific. However, I don't think it is really needed here. Note on that below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116056