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

[Mingming Liu via Phabricator via llvm-commits]

luna added a comment.

thanks!



================
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
----------------
tejohnson wrote:
> 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 ...").
> 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.

I will keep this in mind, thanks for pointing it out!

Removed the comment for function declaration. 




================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1056
+
     if (ExportSummary && (mustBeUnreachableFunction(
                              lookUpFunctionValueInfo(Fn, ExportSummary)))) {
----------------
tejohnson wrote:
> 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.
Got it, thanks for the inputs!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116056