tejohnson added inline comments.
================
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
----------------
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.
================
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
+
----------------
Is there a reason why the hidden version checks after promote but the public
version above checks after importing?
================
Comment at: clang/test/CodeGenCXX/type-metadata.cpp:212
// CFI-NVT-NOT: call i1 @llvm.type.test
- // CFI-VT: [[P:%[^ ]*]] = call i1 @llvm.type.test
+ // CFI-VT-ITANIUM: [[P:%[^ ]*]] = call i1 @llvm.type.test
+ // CFI-VT-MS: [[P:%[^ ]*]] = call i1 @llvm.type.test
----------------
Is the CFI-VT-ITANIUM and CFI-VT-MS split needed? I don't see any differences
in their checking. Can we go back to just a single CFI-VT here?
================
Comment at: lld/test/ELF/lto/update_public_type_test.ll:1
+; REQUIRES: x86
+
----------------
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.
================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:169
+ case Intrinsic::public_type_test: {
+ // errs() << "HIHI " << CI->getParent()->getParent()->getName() <<"\n";
+ // CI->dump();
----------------
Remove commented debugging code here and below
================
Comment at: llvm/lib/LTO/LTO.cpp:1106
DynamicExportSymbols);
+ updatePublicTypeTestCalls(*RegularLTO.CombinedModule,
+ Conf.HasWholeProgramVisibility);
----------------
Probably need to do for the legacy LTO API as well, which also calls
updateVCallVisibilityInModule (LTOCodeGenerator.cpp). This is used both some
other linkers such as ld64. Otherwise I think the public.type.test intrinsics
will never get converted for those LTO users, leading to errors probably? You
can test the legacy LTO via llvm-lto (e.g. see tests in llvm/test/LTO/X86).
================
Comment at: llvm/lib/LTO/LTOBackend.cpp:565
+ updatePublicTypeTestCalls(Mod,
+ Conf.HasWholeProgramVisibility ||
+ CombinedIndex.withWholeProgramVisibility());
----------------
Can we just check the combined index flag now?
================
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.
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.
================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1849
+ dropTypeTests(M, *TypeTestFunc);
+ Function *PublicTypeTestFunc =
+ M.getFunction(Intrinsic::getName(Intrinsic::public_type_test));
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128955/new/
https://reviews.llvm.org/D128955
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
