tejohnson marked an inline comment as done.
tejohnson 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@@"
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71913/new/
https://reviews.llvm.org/D71913
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
