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

Tue Jul 14 20:22:01 PDT 2020

tejohnson marked an inline comment as done.
tejohnson added inline comments.
Herald added a reviewer: MaskRay.

================
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:
> pcc wrote:
> > tejohnson wrote:
> > > pcc wrote:
> > > > 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.
> > > To clarify, from your first paragraph:
> > > 
> > > > 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.
> > > 
> > > Are we currently doing the wrong thing on Windows with -lto-whole-program-visibility, or are you saying it is ineffective anyway? I am not very familiar with Windows linking behavior.
> > > 
> > > > I would be inclined to remove this support
> > > 
> > > Which support are you referring to here? I initially thought you meant my change discussed above to skip adding the type tests in the -flto-visibility-public-std case, but reading the rest of the sentence below I am not so sure I follow.
> > > 
> > > > 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.
> > > Are we currently doing the wrong thing on Windows with -lto-whole-program-visibility, or are you saying it is ineffective anyway? I am not very familiar with Windows linking behavior.
> > 
> > I'm saying that it is ineffective. Windows linkers don't have `-lto-whole-program-visibility`, so either way the class would end up with public LTO visibility.
> > 
> > > Which support are you referring to here? I initially thought you meant my change discussed above to skip adding the type tests in the -flto-visibility-public-std case
> > 
> > Yes, that is what I meant. Sorry if I wasn't clear. That would give us the behaviour of the first option that I mentioned, which would be a consequence of treating classes in `std` the same way as other classes. My second option would be to do that as well as making the compiler omit the type tests for all public LTO visibility classes when targeting Windows (not just the classes in `std` when `-flto-visibility-public-std` is passed), but that's more of an optimization.
> Ok, I am inclined to simply remove this special handling then, which goes back to the original intent which is that type tests don't imply visibility. This allows their use in follow on cases such as guiding ICP with whole program class hierarchy or type test info. I'll send a patch as soon as I get a chance.
> Ok, I am inclined to simply remove this special handling then, which goes back to the original intent which is that type tests don't imply visibility. This allows their use in follow on cases such as guiding ICP with whole program class hierarchy or type test info. I'll send a patch as soon as I get a chance.

Sorry, it took a lot longer to come back and make this change than I intended. Fix mailed in D83845.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71913