aeubanks added a comment. 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 ================ Comment at: clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp:22 // OPT-NOT: @llvm.type.test -// OPT-NOT: call void @llvm.assume // We should have only one @llvm.assume call, the one that was expanded ---------------- tejohnson wrote: > aeubanks wrote: > > tejohnson wrote: > > > Why remove this one here and below? > > we end up RAUWing the `public.type.test` with true and end up with > > `assume(true)` which is harmless and gets removed by something like > > instcombine > Sure, but it should still be gone and can confirm that here? If it isn't > useful to check for, then the comment should probably be changed too since it > mentions assumes. There's an `-O0` RUN line below where the `assume(true)` won't be removed. Updated comments. ================ Comment at: clang/test/CodeGenCXX/thinlto_public_type_test_distributed.ll:23 +; RUN: -o %t.native.o -x ir %t.o --save-temps=obj +; RUN: llvm-dis %t.native.o.1.promote.bc -o - | FileCheck %s --check-prefix=HIDDEN + ---------------- tejohnson wrote: > Is there a reason why the hidden version checks after promote but the public > version above checks after importing? unintentional, fixed. I've changed everything to check 0.preopt since 1.promote wasn't appearing in some cases, not sure why ================ Comment at: lld/test/ELF/lto/update_public_type_test.ll:1 +; REQUIRES: x86 + ---------------- tejohnson wrote: > aeubanks wrote: > > tejohnson wrote: > > > Add comments about what is being tested. Also good to test via llvm-lto2 > > > which has a similar -whole-program-visibility option, rather than just > > > via lld here. See for example > > > llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll. > > added comments > > > > I extended llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll, is that ok? > Yeah sure that's fine. But maybe also add a direct check there in both cases > after promote that the public.type.test/assume were transformed as expected > as you do in this test. done ================ Comment at: llvm/lib/LTO/LTOBackend.cpp:595 + updatePublicTypeTestCalls(Mod, Conf.HasWholeProgramVisibility); + ---------------- 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` ================ Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1849 + dropTypeTests(M, *TypeTestFunc); + Function *PublicTypeTestFunc = + M.getFunction(Intrinsic::getName(Intrinsic::public_type_test)); ---------------- tejohnson wrote: > aeubanks wrote: > > tejohnson wrote: > > > Shouldn't we have converted all of these by now? > > Hmm, I vaguely remember adding this because some clang test failed > > (something to do with the extra LTT we add in BackendUtil.cpp), but > > reverting this doesn't cause any failures. > Related to some of my other comments about ensuring we do the conversion for > the legacy LTO interfaces, perhaps this we should assert here if there are > any remaining public.type.test intrinsics? Not sure what the failure mode is > if any of those stick around otherwise. added a check 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
