aeubanks 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
----------------
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
================
Comment at: clang/test/CodeGenCXX/type-metadata.cpp:148
+ // CFI-TT-MS: [[P:%[^ ]*]] = call i1 @llvm.type.test(i8* [[VT:%[^ ]*]],
metadata !"?AUA@@")
+ // VTABLE-TT-MS: [[P:%[^ ]*]] = call i1 @llvm.type.test(i8* [[VT:%[^ ]*]],
metadata !"?AUA@@")
// TC-ITANIUM: [[PAIR:%[^ ]*]] = call { i8*, i1 }
@llvm.type.checked.load(i8* {{%[^ ]*}}, i32 0, metadata !"_ZTS1A")
----------------
tejohnson wrote:
> It's a little confusing to distinguish the results by prefixing them as "CFI"
> or "VTABLE". Afaict the behavior is determined by whether -fvisibility=hidden
> has been passed in all cases for Itanium.
>
> And MS is always hidden from what I see here? So in that case no need to
> split the expected results into two copies.
not sure why I had CFI and WPD as acting differently in my mind, fixed
================
Comment at: lld/test/ELF/lto/update_public_type_test.ll:1
+; REQUIRES: x86
+
----------------
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?
================
Comment at: llvm/lib/LTO/LTOBackend.cpp:595
+ updatePublicTypeTestCalls(Mod, Conf.HasWholeProgramVisibility);
+
----------------
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.
================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1849
+ dropTypeTests(M, *TypeTestFunc);
+ Function *PublicTypeTestFunc =
+ M.getFunction(Intrinsic::getName(Intrinsic::public_type_test));
----------------
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.
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
