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

[Mingming Liu via Phabricator via llvm-commits]

luna added a comment.

thanks for the simplification!

The comment for IR (test case) is just a FYI, and for the cpp change, more of a double check since I wasn't sure myself if function declaration would make a difference.



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


================
Comment at: llvm/test/ThinLTO/X86/Inputs/devirt_hybrid_after_filtering_unreachable_lib.ll:37-39
-  %this.addr = alloca %class.Derived*, align 8
-  store %class.Derived* %this, %class.Derived** %this.addr, align 8
-  %this1 = load %class.Derived*, %class.Derived** %this.addr, align 8
----------------
thanks for the simplification!

The change looks good to me, and I can verify that nodevirtualization happens on simplified IRs without the fix (by using `-vv` on `llvm-lit` to print out the commands, and re-run commands using {`opt`, `llvm-lto2`} without the change.

Just fyi I added `-O3` (which is absent before) on the command to generate the IR, and got simpler IR for both IR [1]; so going to carry on the simplification in a pending patch (https://reviews.llvm.org/D115648)

[1] https://gist.github.com/minglotus-6/aea3355e6e9a63d5a047a301dca7c7d7 for devirt_hybrid_after_filtering_unreachable.ll, and https://gist.github.com/minglotus-6/546e2a6627a8cc6823022b4ff30a7d43 for Inputs/devirt_hybrid_after_filtering_unreachable_lib.ll,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115780