tejohnson marked 5 inline comments 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:
> > > 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.
================
Comment at: clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp:7
+// as expected.
+// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux
-fvisibility hidden -fwhole-program-vtables -emit-llvm-bc -o %t.o %s
+// RUN: llvm-dis %t.o -o - | FileCheck --check-prefix=TT %s
----------------
evgeny777 wrote:
> I think, we no longer need `-fvisibility hidden` here, do we?
Right, we aren't relying on the visibility for this test which is just ensuring
the type tests are lowered properly in the distributed backends. I removed it.
================
Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:976
+ // via the internal option. Must be done before WPD below.
+ updateVCallVisibilityInIndex(*Index);
+
----------------
evgeny777 wrote:
> evgeny777 wrote:
> > ditto
> updateVCallVisibilityInIndex(*Index, /* WholeProgramVisibilityEnabledInLTO */
> false) ?
Missed this one before (did the InModule version but not InIndex), fixed.
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
