aeubanks added a comment. In D128955#3666995 <https://reviews.llvm.org/D128955#3666995>, @tejohnson wrote:
> 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. I went back to my previous implementation of doing it LTT since we're dropping normal type tests there anyway, and added a comment about this ================ Comment at: llvm/lib/LTO/LTOBackend.cpp:595 + updatePublicTypeTestCalls(Mod, Conf.HasWholeProgramVisibility); + ---------------- tejohnson wrote: > 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). Added both `updatePublicTypeTestCalls` and `setWithWholeProgramVisibility` to `ThinLTOCodeGenerator.cpp`, which `public-type-test.ll` tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128955/new/ https://reviews.llvm.org/D128955 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
