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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks for implementing this!

In D71913 <https://reviews.llvm.org/D71913>, I also modified this code, which might need something similar: https://github.com/llvm/llvm-project/blob/59afc4038b1096bc6fea7b322091f6e5e2dc0b38/clang/lib/CodeGen/ItaniumCXXABI.cpp#L707-L713



================
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
----------------
Why remove this one here and below?


================
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")
----------------
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.


================
Comment at: lld/test/ELF/lto/update_public_type_test.ll:1
+; REQUIRES: x86
+
----------------
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.


================
Comment at: llvm/lib/LTO/LTOBackend.cpp:595
 
+  updatePublicTypeTestCalls(Mod, Conf.HasWholeProgramVisibility);
+
----------------
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)


================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1849
+      dropTypeTests(M, *TypeTestFunc);
+    Function *PublicTypeTestFunc =
+        M.getFunction(Intrinsic::getName(Intrinsic::public_type_test));
----------------
Shouldn't we have converted all of these by now?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128955/new/

https://reviews.llvm.org/D128955