[PATCH] D115780: [LTO][WPD] Simplify mustBeUnreachableFunction and test after D115492

[Mingming Liu via Phabricator via llvm-commits]

luna accepted this revision.
luna added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:242
 // be unreachable but not covered by this helper function.
 static bool mustBeUnreachableFunction(const Function &F) {
+  // A function must be unreachable if its entry block ends with an
----------------
luna wrote:
> > An well-formed IR function definition must have an entry basic block and
> a well-formed IR basic block must have one terminator so the emptiness
> check can be simplified.
> 
> Thanks for pointing this out!
> 
> I think this is true for function definition but not true for function declarations. Yet I don't know if `computeFunctionSummary` [1] (caller of `mustBeUnreachableFunction`) guarantees it's a function definition (not a function declaration).
> 
> Is there a way to make this safe for function declarations (if necessary)?
> 
> [1] https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp#L257
> > An well-formed IR function definition must have an entry basic block and
> a well-formed IR basic block must have one terminator so the emptiness
> check can be simplified.
> 
> Thanks for pointing this out!
> 
> I think this is true for function definition but not true for function declarations. Yet I don't know if `computeFunctionSummary` [1] (caller of `mustBeUnreachableFunction`) guarantees it's a function definition (not a function declaration).
> 
> Is there a way to make this safe for function declarations (if necessary)?
> 
> [1] https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp#L257

After another look, caller guarantees `F` is a function definition, so LGTM. Maybe add a comment for `mustBeUnreachableFunction` that caller guarantees `F` is a function definition (not a function declaration)?

- There is only one caller of `computeFunctionSummary`, and it skips function declarations (https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp#L793)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115780