[PATCH] D149446: [Pipelines] Don't skip GlobalDCE in ThinLTO pre-link

[Mingming Liu via Phabricator via llvm-commits]

mingmingl added inline comments.


================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:1116
-  // Remove dead code, except in the ThinLTO pre-link pipeline where we may want
-  // to keep available_externally functions.
-  if (Phase != ThinOrFullLTOPhase::ThinLTOPreLink)
----------------
mingmingl wrote:
> mingmingl wrote:
> > mingmingl wrote:
> > > This comment reminds me of another comment https://github.com/llvm/llvm-project/blob/4b80a8c72d3d0dba4ee2469471dfccb7e257b319/llvm/lib/Passes/PassBuilderPipelines.cpp#L983-l986, which points out in postlink (after function import) available_externally functions may appear unreferenced, if the function is referenced as virtual table indirect calls, so ICP pass runs before global-opt pass.
> > > 
> > > I don't know how functions become `available_externally` symbols in prelink (e.g., a small c++ example) stage, but wonder if the same 'seemingly unreferenced' but indeed referenced pattern is relevant here.
> > Update:
> > 
> > By flag-gate this pass pipeline change in an application binary (with thinlto and ifdo) and compare the remarks, the counter of remark lines is different (10-ish out of ~30,000 lines removed with prelink global-dce) for ICP. Repeat the flag gate method with WPD remarks gives a small (<0.5%?) counter difference (ignore hash difference in imported function names)
> > 
> > For ICP, the removed remarks is about a function defined in cpp header `a.h` takes a function pointer from `b.cpp`. Need to chase down a little bit to get a reduced test case.
> s/in an application binary/ flag gate the change in compiler to build an application binary with the flag on and off.
Hi, the counter & remark differences turns out to be inside `linkonce_odr` functions and maps back to functions defined in header files (e.g., ref-counted class that invokes virtual destructors of the managed object, etc). After uniquifying the remarks (ignoring counts), the differences looks ok-ish (diffs come from hash suffixes of lto-imported functions). So the change should be benign.

Sorry for the delay, has been looking at this on-and-off.


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

https://reviews.llvm.org/D149446