[PATCH] D128955: [WPD] Use new llvm.public.type.test intrinsic for potentially publicly visible classes

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/LTO/LTOBackend.cpp:595
 
+  updatePublicTypeTestCalls(Mod, Conf.HasWholeProgramVisibility);
+
----------------
aeubanks wrote:
> tejohnson wrote:
> > aeubanks wrote:
> > > I'm not sure where the best place to put these is
> > I don't think that this will work for distributed ThinLTO, where the ThinLTO Backends are run via clang, which isn't going to have this config variable set (since it is set only during LTO linking). I think something may need to be recorded in the index as a flag there at thin link / indexing time that can be checked here.
> > 
> > It would then be nice to consolidate this handling into WPD itself, e.g. at the start of DevirtModule::run, but unfortunately we won't have a summary index for pure regular LTO WPD so I don't think that would work. So in here is ok but I would do it early to handle some of the early return cases.
> > 
> > (Please add a distributed ThinLTO test too)
> Added a distributed ThinLTO test: clang/test/CodeGenCXX/thinlto_public_type_test_distributed.ll.
> 
> I added `ModuleSummaryIndex::WithWholeProgramVisibility` but I'm not sure where I'd set it, and that's causing the test to fail.
ah, I needed a separate WPV flag in llvm-lto2 to set `llvm::lto::Config`, now everything passes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128955