[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added inline comments.


================
Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73
   c1->f();
-  // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2"
+  // ITANIUM: type.test{{.*}}!"_ZTS2C2"
   // MS: type.test{{.*}}!"?AUC2@@"
----------------
tejohnson wrote:
> evgeny777 wrote:
> > tejohnson wrote:
> > > evgeny777 wrote:
> > > > tejohnson wrote:
> > > > > evgeny777 wrote:
> > > > > > What caused this and other changes in this file?
> > > > > Because we now will insert type tests for non-hidden vtables. This is enabled by the changes to LTO to interpret these based on the vcall_visibility metadata.
> > > > The results of this test case
> > > > ```
> > > > %clang_cc1 -flto -triple x86_64-pc-windows-msvc -std=c++11 -fms-extensions -fwhole-program-vtables -flto-visibility-public-std -emit-llvm -o - %s | FileCheck --check-prefix=MS --check-prefix=MS-NOSTD %s
> > > > ```
> > > > look not correct to me. I think you shouldn't generate type tests for standard library classes with  `-flto-visibility-public-std`. Currently if this flag is given, clang doesn't do this either even with `-fvisibility=hidden`
> > > The associated vtables would get the vcall_visibility public metadata, so the type tests themselves aren't problematic. I tend to think that an application using such options should simply stick with -fvisibility=hidden to get WPD and not use the new LTO option to convert all public vcall_visibility metadata to hidden.
> > > The associated vtables would get the vcall_visibility public metadata, so the type tests themselves aren't problematic. I tend to think that an application using such options should simply stick with -fvisibility=hidden to get WPD and not use the new LTO option to convert all public vcall_visibility metadata to hidden.
> > 
> > I see two issues here:
> > 1) It's not always good option to force hidden visibility for everything. For instance I work on proprietary platform which demands public visibility for certain symbols in order for dynamic loader to work properly. In this context your patch does a great job.
> > 
> > 2) Standard library is almost never LTOed so in general we can't narrow std::* vtables visibility to linkage unit
> > 
> > Is there anything which prevents from implementing the same functionality with new -lto-whole-program-visibility option (i.e without forcing hidden visibility)? In other words the following looks good to me:
> > 
> > ```
> > # Compile with lto/devirtualization support
> > clang -flto=thin -flto-visibility-public-std -fwhole-program-vtables -c *.cpp
> > 
> > # Link: everything is devirtualized except standard library classes virtual methods
> > clang -Wl,-lto-whole-program-visibility -fuse-ld=lld *.o
> > ```
> Ok, thanks for the info. I will go ahead and change the code to not insert the type tests in this case to support this usage.
> 
> Ultimately, I would like to decouple the existence of the type tests from visibility implications. I'm working on another change to delay lowering/removal of type tests until after indirect call promotion, so we can use them in other cases (streamlined indirect call promotion checks against the vtable instead of the function pointers, also useful if we want to implement speculative devirtualization based on WPD info). In those cases we need the type tests, either to locate information in the summary, or to get the address point offset for a vtable address compare. In that case it would be helpful to have the type tests in this type of code as well. So we'll need another way to communicate down to WPD that they should never be devirtualized. But I don't think it makes sense to design this support until there is a concrete use case and need. In the meantime I will change the code to be conservative and not insert the type tests in this case.
Note that `-flto-visibility-public-std` is a cc1-only option and only used on Windows, and further that `-lto-whole-program-visibility` as implemented doesn't really make sense on Windows because the classes with public visibility are going to be marked dllimport/dllexport/uuid (COM), and `-lto-whole-program-visibility` corresponds to flags such as `--version-script` or the absence of `-shared` in which the linker automatically relaxes the visibility of some symbols, while there is no such concept of relaxing symbol visibility on Windows.

 I would be inclined to remove this support and either let the public visibility automatically derive from the absence of `-lto-whole-program-visibility` at link time in COFF links or omit the IR needed to support `lto-whole-program-visibility` on Windows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913