[PATCH] D128955: [WPD] Use new llvm.public.type.test intrinsic for potentially publicly visible classes

[Arthur Eubanks via Phabricator via cfe-commits]

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