[PATCH] D155659: [WPD][LLD] Add option to validate RTTI is enabled on all native types and prevent devirtualization on types with native RTTI

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks for the patch. I have some initial comments below. From my reading, I guess any native object with a vtable and no RTTI will disable WPD globally, which is unfortunate. Although I do have a suggestion below for making this slightly less pessimistic. I'm curious to give this a try internally on a few codes and see how frequently it ends up disabling WPD.



================
Comment at: llvm/lib/LTO/LTO.cpp:1705
 
+  auto isVisibleOutsideSummary = [&](StringRef name) {
+    return GlobalResolutions.count(name)
----------------
Would be better to have a more specific name, since this is only queried with type names. I.e. local symbols are not visible outside the summary but don't have a GlobalResolution entry. But you aren't calling this lambda in that case (but that isn't clear, where the lambda is defined). Before I suggest a name, I have a question about the usage of this lambda down in the WPD code.


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:2413
+  // case bail on the optimization.
+  if (ValidateAllVtablesHaveTypeInfos && !AllVtablesHaveTypeInfos)
+    return;
----------------
Rather than doing this down here in LTO/WPD could the linker simply unset the HasWholeProgramVisibility config flag? That would also allow WPD to proceed on types with hidden LTO visibility. This early return would prevent any and all WPD which seems overly conservative in the case of hidden LTO visibility classes. 


================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:2485
+        // Local linkage is always safe
+        // Note that if RTTI is not enabled in LTO unit, the typename
+        // symbol doesn't exist and we don't have a resolution meaning
----------------
Can we ever get here if RTTI is not enabled? My understanding of the change to line 2413 is that we return early in that case. Given that early return, aren't we guaranteed that the typename symbol has a GlobalResolution if it is non-local?

Oh - I guess we are only early returning if RTTI is off in native objects, so you could get here if RTTI is only disabled in bitcode objects? And we need to be conservative for any typenames for vtables defined in bitcode objects with RTTI off? I didn't see a test for this case, can you add one (or did I miss it)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155659