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

Teresa Johnson via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Wed Jul 20 15:59:34 PDT 2022


tejohnson added a comment.

In D128955#3666830 <https://reviews.llvm.org/D128955#3666830>, @aeubanks wrote:

> the assert that there are no public.type.tests in LTT fails on `CodeGenCXX/thinlto-distributed-type-metadata.cpp`. for some reason clang doesn't go through the LTO API at [1], it just ends up calling the normal optimization pipeline. any ideas why?
>
> [1] https://github.com/llvm/llvm-project/blob/0c1b32717bcffcf8edf95294e98933bd4c1e76ed/clang/lib/CodeGen/BackendUtil.cpp#L1173

Ah ok, that handling is for some cases where we may decide not to do ThinLTO, but rather compile the IR down to native code normally without a thin link (we use that feature for distributed build system reasons). Looks like we need a fallback mechanism to convert these in that case. Maybe right at the start of DevirtModule::run? We could do in LTT where you are currently asserting too, but to me it seems more natural to get rid of these at the start of WPD, because with LTO they are gone by then.



================
Comment at: llvm/lib/LTO/LTOBackend.cpp:595
 
+  updatePublicTypeTestCalls(Mod, Conf.HasWholeProgramVisibility);
+
----------------
aeubanks wrote:
> aeubanks wrote:
> > tejohnson wrote:
> > > 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.
> > > I see from your most recent update that you added code to set/check this new index flag. But I think you would want to set it around the same place where we update the vcall visibility during the thin link (see calls to updateVCallVisibilityInIndex). And I would do it via a new method in the WPD source file that uses the existing hasWholeProgramVisibility() that checks the OR of the config flag or the internal option. Then you don't need to add a new flag to llvm-lto2 for this purpose.
> > > 
> > > Also, as I noted for the regular LTO handling elsewhere, I think you need to add similar handling for the legacy LTO implementation. See other callers to updateVCallVisibilityInIndex.
> > ah, I needed a separate WPV flag in llvm-lto2 to set `llvm::lto::Config`, now everything passes
> exposed the existing `hasWholeProgramVisibility`
See note about needing this for legacy LTO API (ThinLTOCodeGenerator.cpp).


================
Comment at: llvm/test/LTO/X86/public-type-test.ll:1
+; Test to ensure that the legacy LTO API lowers @llvm.public.type.test.
+
----------------
Needs some CHECKs?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128955



More information about the cfe-commits mailing list