tejohnson marked 9 inline comments as done. tejohnson added inline comments.
================ Comment at: clang/lib/CodeGen/BackendUtil.cpp:559 + if (!CodeGenOpts.ThinLTOIndexFile.empty()) + MPM.add(createLowerTypeTestsPass(/*ExportSummary=*/nullptr, + /*ImportSummary=*/nullptr, ---------------- evgeny777 wrote: > Test case? See clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp. Specifically the second block of clang invocations: ``` // Also check type test are lowered when the distributed ThinLTO backend clang // invocation is passed an empty index file, in which case a non-ThinLTO // compilation pipeline is invoked. If not lowered then LLVM CodeGen may assert. ``` I'm testing both the new and old PMs, as well as the new PM O0 path (basically all paths modified in this file). ================ Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73 c1->f(); - // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2" + // ITANIUM: type.test{{.*}}!"_ZTS2C2" // MS: type.test{{.*}}!"?AUC2@@" ---------------- evgeny777 wrote: > What caused this and other changes in this file? Because we now will insert type tests for non-hidden vtables. This is enabled by the changes to LTO to interpret these based on the vcall_visibility metadata. ================ Comment at: llvm/include/llvm/Transforms/IPO.h:245 + const ModuleSummaryIndex *ImportSummary, + bool StripAll = false); ---------------- evgeny777 wrote: > s/StripAll/DropTypeTests/g ? Woops, missed this one after my rename! Good catch. Also, noticed the description in the comments is now stale, fixed. ================ Comment at: llvm/lib/LTO/LTOCodeGenerator.cpp:550 + // pipeline run below. + updateVCallVisibilityInModule(*MergedModule); + ---------------- evgeny777 wrote: > I'd rather use > ``` > updateVCallVisibilityInModule(*MergedModule, /* > WholeProgramVisibilityEnabledInLTO */ false) > ``` > and remove default value for second parameter Good idea, changed. ================ Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1775 + if (TypeTestFunc) { + for (auto UI = TypeTestFunc->use_begin(), UE = TypeTestFunc->use_end(); + UI != UE;) { ---------------- evgeny777 wrote: > Fold identical code blocks After looking at this code again, I have changed it a bit. We should only be removing assumes that are consuming type test instructions. I have changed this (which removes the redundancy in any case). I also added a check to the test mentioned earlier for this handling (clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp) to ensure we don't remove unrelated llvm.assume. ================ Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:145 +/// when enabled via the linker. +cl::opt<bool> DisableWholeProgramVisibility( + "disable-whole-program-visibility", cl::init(false), cl::Hidden, ---------------- evgeny777 wrote: > Is this tested? Added a test of it to llvm/test/ThinLTO/X86/devirt_vcall_vis_public.ll. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71913/new/ https://reviews.llvm.org/D71913 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits