[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL292662: IRGen: Start using the WriteThinLTOBitcode pass.
(authored by pcc).
Changed prior to commit:
https://reviews.llvm.org/D28843?vs=84907=85198#toc
Repository:
rL LLVM
https://reviews.llvm.org/D28843
Files:
cfe/trunk/lib/CodeGen/BackendUtil.cpp
cfe/trunk/test/CMakeLists.txt
cfe/trunk/test/CodeGenCXX/type-metadata-thinlto.cpp
Index: cfe/trunk/test/CodeGenCXX/type-metadata-thinlto.cpp
===
--- cfe/trunk/test/CodeGenCXX/type-metadata-thinlto.cpp
+++ cfe/trunk/test/CodeGenCXX/type-metadata-thinlto.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux
-fvisibility hidden -emit-llvm-bc -o %t %s
+// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s
+
+// CHECK: @_ZTV1A = linkonce_odr
+class A {
+ virtual void f() {}
+};
+
+A *f() {
+ return new A;
+}
Index: cfe/trunk/test/CMakeLists.txt
===
--- cfe/trunk/test/CMakeLists.txt
+++ cfe/trunk/test/CMakeLists.txt
@@ -80,6 +80,7 @@
llc
llvm-bcanalyzer
llvm-dis
+llvm-modextract
llvm-nm
llvm-objdump
llvm-profdata
Index: cfe/trunk/lib/CodeGen/BackendUtil.cpp
===
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp
@@ -689,9 +689,11 @@
break;
case Backend_EmitBC:
-PerModulePasses.add(createBitcodeWriterPass(
-*OS, CodeGenOpts.EmitLLVMUseLists, CodeGenOpts.EmitSummaryIndex,
-CodeGenOpts.EmitSummaryIndex));
+if (CodeGenOpts.EmitSummaryIndex)
+ PerModulePasses.add(createWriteThinLTOBitcodePass(*OS));
+else
+ PerModulePasses.add(
+ createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists));
break;
case Backend_EmitLL:
Index: cfe/trunk/test/CodeGenCXX/type-metadata-thinlto.cpp
===
--- cfe/trunk/test/CodeGenCXX/type-metadata-thinlto.cpp
+++ cfe/trunk/test/CodeGenCXX/type-metadata-thinlto.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
+// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s
+
+// CHECK: @_ZTV1A = linkonce_odr
+class A {
+ virtual void f() {}
+};
+
+A *f() {
+ return new A;
+}
Index: cfe/trunk/test/CMakeLists.txt
===
--- cfe/trunk/test/CMakeLists.txt
+++ cfe/trunk/test/CMakeLists.txt
@@ -80,6 +80,7 @@
llc
llvm-bcanalyzer
llvm-dis
+llvm-modextract
llvm-nm
llvm-objdump
llvm-profdata
Index: cfe/trunk/lib/CodeGen/BackendUtil.cpp
===
--- cfe/trunk/lib/CodeGen/BackendUtil.cpp
+++ cfe/trunk/lib/CodeGen/BackendUtil.cpp
@@ -689,9 +689,11 @@
break;
case Backend_EmitBC:
-PerModulePasses.add(createBitcodeWriterPass(
-*OS, CodeGenOpts.EmitLLVMUseLists, CodeGenOpts.EmitSummaryIndex,
-CodeGenOpts.EmitSummaryIndex));
+if (CodeGenOpts.EmitSummaryIndex)
+ PerModulePasses.add(createWriteThinLTOBitcodePass(*OS));
+else
+ PerModulePasses.add(
+ createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists));
break;
case Backend_EmitLL:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
pcc updated this revision to Diff 84907.
pcc added a comment.
Herald added a subscriber: mgorny.
- Add missing test dependency
https://reviews.llvm.org/D28843
Files:
clang/lib/CodeGen/BackendUtil.cpp
clang/test/CMakeLists.txt
clang/test/CodeGenCXX/type-metadata-thinlto.cpp
Index: clang/test/CodeGenCXX/type-metadata-thinlto.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/type-metadata-thinlto.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux
-fvisibility hidden -emit-llvm-bc -o %t %s
+// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s
+
+// CHECK: @_ZTV1A = linkonce_odr
+class A {
+ virtual void f() {}
+};
+
+A *f() {
+ return new A;
+}
Index: clang/test/CMakeLists.txt
===
--- clang/test/CMakeLists.txt
+++ clang/test/CMakeLists.txt
@@ -80,6 +80,7 @@
llc
llvm-bcanalyzer
llvm-dis
+llvm-modextract
llvm-nm
llvm-objdump
llvm-profdata
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -688,9 +688,11 @@
break;
case Backend_EmitBC:
-PerModulePasses.add(createBitcodeWriterPass(
-*OS, CodeGenOpts.EmitLLVMUseLists, CodeGenOpts.EmitSummaryIndex,
-CodeGenOpts.EmitSummaryIndex));
+if (CodeGenOpts.EmitSummaryIndex)
+ PerModulePasses.add(createWriteThinLTOBitcodePass(*OS));
+else
+ PerModulePasses.add(
+ createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists));
break;
case Backend_EmitLL:
Index: clang/test/CodeGenCXX/type-metadata-thinlto.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/type-metadata-thinlto.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
+// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s
+
+// CHECK: @_ZTV1A = linkonce_odr
+class A {
+ virtual void f() {}
+};
+
+A *f() {
+ return new A;
+}
Index: clang/test/CMakeLists.txt
===
--- clang/test/CMakeLists.txt
+++ clang/test/CMakeLists.txt
@@ -80,6 +80,7 @@
llc
llvm-bcanalyzer
llvm-dis
+llvm-modextract
llvm-nm
llvm-objdump
llvm-profdata
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -688,9 +688,11 @@
break;
case Backend_EmitBC:
-PerModulePasses.add(createBitcodeWriterPass(
-*OS, CodeGenOpts.EmitLLVMUseLists, CodeGenOpts.EmitSummaryIndex,
-CodeGenOpts.EmitSummaryIndex));
+if (CodeGenOpts.EmitSummaryIndex)
+ PerModulePasses.add(createWriteThinLTOBitcodePass(*OS));
+else
+ PerModulePasses.add(
+ createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists));
break;
case Backend_EmitLL:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D28843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
pcc updated this revision to Diff 84903.
pcc added a comment.
Refresh
https://reviews.llvm.org/D28843
Files:
clang/lib/CodeGen/BackendUtil.cpp
clang/test/CodeGenCXX/type-metadata-thinlto.cpp
Index: clang/test/CodeGenCXX/type-metadata-thinlto.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/type-metadata-thinlto.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux
-fvisibility hidden -emit-llvm-bc -o %t %s
+// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s
+
+// CHECK: @_ZTV1A = linkonce_odr
+class A {
+ virtual void f() {}
+};
+
+A *f() {
+ return new A;
+}
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -688,9 +688,11 @@
break;
case Backend_EmitBC:
-PerModulePasses.add(createBitcodeWriterPass(
-*OS, CodeGenOpts.EmitLLVMUseLists, CodeGenOpts.EmitSummaryIndex,
-CodeGenOpts.EmitSummaryIndex));
+if (CodeGenOpts.EmitSummaryIndex)
+ PerModulePasses.add(createWriteThinLTOBitcodePass(*OS));
+else
+ PerModulePasses.add(
+ createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists));
break;
case Backend_EmitLL:
Index: clang/test/CodeGenCXX/type-metadata-thinlto.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/type-metadata-thinlto.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -flto=thin -flto-unit -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
+// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s
+
+// CHECK: @_ZTV1A = linkonce_odr
+class A {
+ virtual void f() {}
+};
+
+A *f() {
+ return new A;
+}
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -688,9 +688,11 @@
break;
case Backend_EmitBC:
-PerModulePasses.add(createBitcodeWriterPass(
-*OS, CodeGenOpts.EmitLLVMUseLists, CodeGenOpts.EmitSummaryIndex,
-CodeGenOpts.EmitSummaryIndex));
+if (CodeGenOpts.EmitSummaryIndex)
+ PerModulePasses.add(createWriteThinLTOBitcodePass(*OS));
+else
+ PerModulePasses.add(
+ createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists));
break;
case Backend_EmitLL:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
pcc added inline comments. Comment at: clang/test/CodeGenCXX/type-metadata-thinlto.cpp:2 +// RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s +// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s + tejohnson wrote: > mehdi_amini wrote: > > tejohnson wrote: > > > mehdi_amini wrote: > > > > pcc wrote: > > > > > mehdi_amini wrote: > > > > > > pcc wrote: > > > > > > > mehdi_amini wrote: > > > > > > > > tejohnson wrote: > > > > > > > > > Is it the case that now we will always split the module with > > > > > > > > > this change? Should that only be done under CFI options? > > > > > > > > Devirtualization may happen whenever you have a hidden virtual > > > > > > > > table IIUC, independently of CFI. > > > > > > > To be more precise: we normally add type metadata in LTO mode > > > > > > > when the class has hidden visibility. See: > > > > > > > http://clang.llvm.org/docs/LTOVisibility.html > > > > > > > > > > > > > > That doesn't necessarily imply devirtualization, which is > > > > > > > controlled by the flag `-fwhole-program-vtables`. > > > > > > So with hidden visibility but without CFI or > > > > > > -fwhole-program-vtables, do we split the module? What's the purpose? > > > > > At the moment we would. The purpose is to simplify the overall > > > > > interface. If I want to compile a subset of my TUs without CFI or > > > > > devirtualization, I should be able to do that by enabling LTO but not > > > > > passing the CFI or devirtualization flags. In that case the vtables > > > > > themselves should still have type metadata so that TUs compiled > > > > > without CFI/devirtualization can correctly interoperate with TUs > > > > > compiled with CFI/devirtualization (to handle the cases where a class > > > > > defined in a TU compiled without CFI/devirt is used by code compiled > > > > > with LTO/devirt, or where the linker/LTO selects a linkonce_odr > > > > > vtable from a TU compiled without CFI/devirt). > > > > > > > > > > I'd be open to changing the command line interface so that an > > > > > additional flag may be used to control the scope of the "LTO unit" > > > > > and which would just enable type metadata, but ideally I'd like to > > > > > keep things relatively simple. > > > > > At the moment we would. The purpose is to simplify the overall > > > > > interface. > > > > > > > > Right, if it was the only reason, I wouldn't be in favor, but you raise > > > > a real use case below. > > > > > > > > > If I want to compile a subset of my TUs without CFI or > > > > > devirtualization, I should be able to do that by enabling LTO but not > > > > > passing the CFI or devirtualization flags. > > > > > > > > Right, seems legit. > > > > > > > > > In that case the vtables themselves should still have type metadata > > > > > so that TUs compiled without CFI/devirtualization can correctly > > > > > interoperate with TUs compiled with CFI/devirtualization > > > > > > > > That's what I wasn't sure about :) > > > > > > > > > (to handle the cases where a class defined in a TU compiled without > > > > > CFI/devirt is used by code compiled with LTO/devirt, or where the > > > > > linker/LTO selects a linkonce_odr vtable from a TU compiled without > > > > > CFI/devirt). > > > > > > > > Make sense, LGTM as is with this explanation! > > > > Thanks. > > > I would like some way to disable this, at least for debugging. > > CC1 option then? > > CC1 option then? > > Sure D28877 https://reviews.llvm.org/D28843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
tejohnson added inline comments. Comment at: clang/test/CodeGenCXX/type-metadata-thinlto.cpp:2 +// RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s +// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s + mehdi_amini wrote: > tejohnson wrote: > > mehdi_amini wrote: > > > pcc wrote: > > > > mehdi_amini wrote: > > > > > pcc wrote: > > > > > > mehdi_amini wrote: > > > > > > > tejohnson wrote: > > > > > > > > Is it the case that now we will always split the module with > > > > > > > > this change? Should that only be done under CFI options? > > > > > > > Devirtualization may happen whenever you have a hidden virtual > > > > > > > table IIUC, independently of CFI. > > > > > > To be more precise: we normally add type metadata in LTO mode when > > > > > > the class has hidden visibility. See: > > > > > > http://clang.llvm.org/docs/LTOVisibility.html > > > > > > > > > > > > That doesn't necessarily imply devirtualization, which is > > > > > > controlled by the flag `-fwhole-program-vtables`. > > > > > So with hidden visibility but without CFI or -fwhole-program-vtables, > > > > > do we split the module? What's the purpose? > > > > At the moment we would. The purpose is to simplify the overall > > > > interface. If I want to compile a subset of my TUs without CFI or > > > > devirtualization, I should be able to do that by enabling LTO but not > > > > passing the CFI or devirtualization flags. In that case the vtables > > > > themselves should still have type metadata so that TUs compiled without > > > > CFI/devirtualization can correctly interoperate with TUs compiled with > > > > CFI/devirtualization (to handle the cases where a class defined in a TU > > > > compiled without CFI/devirt is used by code compiled with LTO/devirt, > > > > or where the linker/LTO selects a linkonce_odr vtable from a TU > > > > compiled without CFI/devirt). > > > > > > > > I'd be open to changing the command line interface so that an > > > > additional flag may be used to control the scope of the "LTO unit" and > > > > which would just enable type metadata, but ideally I'd like to keep > > > > things relatively simple. > > > > At the moment we would. The purpose is to simplify the overall > > > > interface. > > > > > > Right, if it was the only reason, I wouldn't be in favor, but you raise a > > > real use case below. > > > > > > > If I want to compile a subset of my TUs without CFI or > > > > devirtualization, I should be able to do that by enabling LTO but not > > > > passing the CFI or devirtualization flags. > > > > > > Right, seems legit. > > > > > > > In that case the vtables themselves should still have type metadata so > > > > that TUs compiled without CFI/devirtualization can correctly > > > > interoperate with TUs compiled with CFI/devirtualization > > > > > > That's what I wasn't sure about :) > > > > > > > (to handle the cases where a class defined in a TU compiled without > > > > CFI/devirt is used by code compiled with LTO/devirt, or where the > > > > linker/LTO selects a linkonce_odr vtable from a TU compiled without > > > > CFI/devirt). > > > > > > Make sense, LGTM as is with this explanation! > > > Thanks. > > I would like some way to disable this, at least for debugging. > CC1 option then? > CC1 option then? Sure https://reviews.llvm.org/D28843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
mehdi_amini added inline comments. Comment at: clang/test/CodeGenCXX/type-metadata-thinlto.cpp:2 +// RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s +// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s + tejohnson wrote: > mehdi_amini wrote: > > pcc wrote: > > > mehdi_amini wrote: > > > > pcc wrote: > > > > > mehdi_amini wrote: > > > > > > tejohnson wrote: > > > > > > > Is it the case that now we will always split the module with this > > > > > > > change? Should that only be done under CFI options? > > > > > > Devirtualization may happen whenever you have a hidden virtual > > > > > > table IIUC, independently of CFI. > > > > > To be more precise: we normally add type metadata in LTO mode when > > > > > the class has hidden visibility. See: > > > > > http://clang.llvm.org/docs/LTOVisibility.html > > > > > > > > > > That doesn't necessarily imply devirtualization, which is controlled > > > > > by the flag `-fwhole-program-vtables`. > > > > So with hidden visibility but without CFI or -fwhole-program-vtables, > > > > do we split the module? What's the purpose? > > > At the moment we would. The purpose is to simplify the overall interface. > > > If I want to compile a subset of my TUs without CFI or devirtualization, > > > I should be able to do that by enabling LTO but not passing the CFI or > > > devirtualization flags. In that case the vtables themselves should still > > > have type metadata so that TUs compiled without CFI/devirtualization can > > > correctly interoperate with TUs compiled with CFI/devirtualization (to > > > handle the cases where a class defined in a TU compiled without > > > CFI/devirt is used by code compiled with LTO/devirt, or where the > > > linker/LTO selects a linkonce_odr vtable from a TU compiled without > > > CFI/devirt). > > > > > > I'd be open to changing the command line interface so that an additional > > > flag may be used to control the scope of the "LTO unit" and which would > > > just enable type metadata, but ideally I'd like to keep things relatively > > > simple. > > > At the moment we would. The purpose is to simplify the overall interface. > > > > Right, if it was the only reason, I wouldn't be in favor, but you raise a > > real use case below. > > > > > If I want to compile a subset of my TUs without CFI or devirtualization, > > > I should be able to do that by enabling LTO but not passing the CFI or > > > devirtualization flags. > > > > Right, seems legit. > > > > > In that case the vtables themselves should still have type metadata so > > > that TUs compiled without CFI/devirtualization can correctly interoperate > > > with TUs compiled with CFI/devirtualization > > > > That's what I wasn't sure about :) > > > > > (to handle the cases where a class defined in a TU compiled without > > > CFI/devirt is used by code compiled with LTO/devirt, or where the > > > linker/LTO selects a linkonce_odr vtable from a TU compiled without > > > CFI/devirt). > > > > Make sense, LGTM as is with this explanation! > > Thanks. > I would like some way to disable this, at least for debugging. CC1 option then? https://reviews.llvm.org/D28843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
tejohnson added inline comments. Comment at: clang/test/CodeGenCXX/type-metadata-thinlto.cpp:2 +// RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s +// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s + mehdi_amini wrote: > pcc wrote: > > mehdi_amini wrote: > > > pcc wrote: > > > > mehdi_amini wrote: > > > > > tejohnson wrote: > > > > > > Is it the case that now we will always split the module with this > > > > > > change? Should that only be done under CFI options? > > > > > Devirtualization may happen whenever you have a hidden virtual table > > > > > IIUC, independently of CFI. > > > > To be more precise: we normally add type metadata in LTO mode when the > > > > class has hidden visibility. See: > > > > http://clang.llvm.org/docs/LTOVisibility.html > > > > > > > > That doesn't necessarily imply devirtualization, which is controlled by > > > > the flag `-fwhole-program-vtables`. > > > So with hidden visibility but without CFI or -fwhole-program-vtables, do > > > we split the module? What's the purpose? > > At the moment we would. The purpose is to simplify the overall interface. > > If I want to compile a subset of my TUs without CFI or devirtualization, I > > should be able to do that by enabling LTO but not passing the CFI or > > devirtualization flags. In that case the vtables themselves should still > > have type metadata so that TUs compiled without CFI/devirtualization can > > correctly interoperate with TUs compiled with CFI/devirtualization (to > > handle the cases where a class defined in a TU compiled without CFI/devirt > > is used by code compiled with LTO/devirt, or where the linker/LTO selects a > > linkonce_odr vtable from a TU compiled without CFI/devirt). > > > > I'd be open to changing the command line interface so that an additional > > flag may be used to control the scope of the "LTO unit" and which would > > just enable type metadata, but ideally I'd like to keep things relatively > > simple. > > At the moment we would. The purpose is to simplify the overall interface. > > Right, if it was the only reason, I wouldn't be in favor, but you raise a > real use case below. > > > If I want to compile a subset of my TUs without CFI or devirtualization, I > > should be able to do that by enabling LTO but not passing the CFI or > > devirtualization flags. > > Right, seems legit. > > > In that case the vtables themselves should still have type metadata so that > > TUs compiled without CFI/devirtualization can correctly interoperate with > > TUs compiled with CFI/devirtualization > > That's what I wasn't sure about :) > > > (to handle the cases where a class defined in a TU compiled without > > CFI/devirt is used by code compiled with LTO/devirt, or where the > > linker/LTO selects a linkonce_odr vtable from a TU compiled without > > CFI/devirt). > > Make sense, LGTM as is with this explanation! > Thanks. I would like some way to disable this, at least for debugging. https://reviews.llvm.org/D28843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
mehdi_amini added inline comments. Comment at: clang/test/CodeGenCXX/type-metadata-thinlto.cpp:2 +// RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s +// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s + pcc wrote: > mehdi_amini wrote: > > pcc wrote: > > > mehdi_amini wrote: > > > > tejohnson wrote: > > > > > Is it the case that now we will always split the module with this > > > > > change? Should that only be done under CFI options? > > > > Devirtualization may happen whenever you have a hidden virtual table > > > > IIUC, independently of CFI. > > > To be more precise: we normally add type metadata in LTO mode when the > > > class has hidden visibility. See: > > > http://clang.llvm.org/docs/LTOVisibility.html > > > > > > That doesn't necessarily imply devirtualization, which is controlled by > > > the flag `-fwhole-program-vtables`. > > So with hidden visibility but without CFI or -fwhole-program-vtables, do we > > split the module? What's the purpose? > At the moment we would. The purpose is to simplify the overall interface. If > I want to compile a subset of my TUs without CFI or devirtualization, I > should be able to do that by enabling LTO but not passing the CFI or > devirtualization flags. In that case the vtables themselves should still have > type metadata so that TUs compiled without CFI/devirtualization can correctly > interoperate with TUs compiled with CFI/devirtualization (to handle the cases > where a class defined in a TU compiled without CFI/devirt is used by code > compiled with LTO/devirt, or where the linker/LTO selects a linkonce_odr > vtable from a TU compiled without CFI/devirt). > > I'd be open to changing the command line interface so that an additional flag > may be used to control the scope of the "LTO unit" and which would just > enable type metadata, but ideally I'd like to keep things relatively simple. > At the moment we would. The purpose is to simplify the overall interface. Right, if it was the only reason, I wouldn't be in favor, but you raise a real use case below. > If I want to compile a subset of my TUs without CFI or devirtualization, I > should be able to do that by enabling LTO but not passing the CFI or > devirtualization flags. Right, seems legit. > In that case the vtables themselves should still have type metadata so that > TUs compiled without CFI/devirtualization can correctly interoperate with TUs > compiled with CFI/devirtualization That's what I wasn't sure about :) > (to handle the cases where a class defined in a TU compiled without > CFI/devirt is used by code compiled with LTO/devirt, or where the linker/LTO > selects a linkonce_odr vtable from a TU compiled without CFI/devirt). Make sense, LGTM as is with this explanation! Thanks. https://reviews.llvm.org/D28843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
pcc added inline comments. Comment at: clang/test/CodeGenCXX/type-metadata-thinlto.cpp:2 +// RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s +// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s + mehdi_amini wrote: > pcc wrote: > > mehdi_amini wrote: > > > tejohnson wrote: > > > > Is it the case that now we will always split the module with this > > > > change? Should that only be done under CFI options? > > > Devirtualization may happen whenever you have a hidden virtual table > > > IIUC, independently of CFI. > > To be more precise: we normally add type metadata in LTO mode when the > > class has hidden visibility. See: > > http://clang.llvm.org/docs/LTOVisibility.html > > > > That doesn't necessarily imply devirtualization, which is controlled by the > > flag `-fwhole-program-vtables`. > So with hidden visibility but without CFI or -fwhole-program-vtables, do we > split the module? What's the purpose? At the moment we would. The purpose is to simplify the overall interface. If I want to compile a subset of my TUs without CFI or devirtualization, I should be able to do that by enabling LTO but not passing the CFI or devirtualization flags. In that case the vtables themselves should still have type metadata so that TUs compiled without CFI/devirtualization can correctly interoperate with TUs compiled with CFI/devirtualization (to handle the cases where a class defined in a TU compiled without CFI/devirt is used by code compiled with LTO/devirt, or where the linker/LTO selects a linkonce_odr vtable from a TU compiled without CFI/devirt). I'd be open to changing the command line interface so that an additional flag may be used to control the scope of the "LTO unit" and which would just enable type metadata, but ideally I'd like to keep things relatively simple. https://reviews.llvm.org/D28843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
mehdi_amini added inline comments. Comment at: clang/test/CodeGenCXX/type-metadata-thinlto.cpp:2 +// RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s +// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s + pcc wrote: > mehdi_amini wrote: > > tejohnson wrote: > > > Is it the case that now we will always split the module with this change? > > > Should that only be done under CFI options? > > Devirtualization may happen whenever you have a hidden virtual table IIUC, > > independently of CFI. > To be more precise: we normally add type metadata in LTO mode when the class > has hidden visibility. See: http://clang.llvm.org/docs/LTOVisibility.html > > That doesn't necessarily imply devirtualization, which is controlled by the > flag `-fwhole-program-vtables`. So with hidden visibility but without CFI or -fwhole-program-vtables, do we split the module? What's the purpose? https://reviews.llvm.org/D28843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
pcc added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:694 +else + PerModulePasses.add( + createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists)); tejohnson wrote: > Can we transform other callers of createBitcodeWriterPass with > EmitSummaryIndex = true or EmitModuleHash = true to instead call > createWriteThinLTOBitcodePass as you did here, and then remove those > parameters from createBitcodeWriterPass? http://llvm-cs.pcc.me.uk/lib/Bitcode/Writer/BitcodeWriterPass.cpp/rcreateBitcodeWriterPass The only other caller like that is in opt. I think that if we change the "under the hood" parts of opt we should change it to not use a pass at all (c.f. our earlier discussion on D27324), then remove those parameters. Comment at: clang/test/CodeGenCXX/type-metadata-thinlto.cpp:2 +// RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s +// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s + mehdi_amini wrote: > tejohnson wrote: > > Is it the case that now we will always split the module with this change? > > Should that only be done under CFI options? > Devirtualization may happen whenever you have a hidden virtual table IIUC, > independently of CFI. To be more precise: we normally add type metadata in LTO mode when the class has hidden visibility. See: http://clang.llvm.org/docs/LTOVisibility.html That doesn't necessarily imply devirtualization, which is controlled by the flag `-fwhole-program-vtables`. https://reviews.llvm.org/D28843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
mehdi_amini added inline comments. Comment at: clang/test/CodeGenCXX/type-metadata-thinlto.cpp:2 +// RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s +// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s + tejohnson wrote: > Is it the case that now we will always split the module with this change? > Should that only be done under CFI options? Devirtualization may happen whenever you have a hidden virtual table IIUC, independently of CFI. https://reviews.llvm.org/D28843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
mehdi_amini added a comment. In https://reviews.llvm.org/D28843#649207, @tejohnson wrote: > > TODO: avoid breaking Darwin. > > How does this break Darwin? IIUC, we don't use the new LTO API, which is handling this gracefully. https://reviews.llvm.org/D28843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
tejohnson added a comment. > TODO: avoid breaking Darwin. How does this break Darwin? Comment at: clang/lib/CodeGen/BackendUtil.cpp:694 +else + PerModulePasses.add( + createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists)); Can we transform other callers of createBitcodeWriterPass with EmitSummaryIndex = true or EmitModuleHash = true to instead call createWriteThinLTOBitcodePass as you did here, and then remove those parameters from createBitcodeWriterPass? Comment at: clang/test/CodeGenCXX/type-metadata-thinlto.cpp:2 +// RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s +// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s + Is it the case that now we will always split the module with this change? Should that only be done under CFI options? https://reviews.llvm.org/D28843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.
pcc created this revision.
This is the final change necessary to support CFI with ThinLTO.
TODO: avoid breaking Darwin.
Depends on https://reviews.llvm.org/D28840
https://reviews.llvm.org/D28843
Files:
clang/lib/CodeGen/BackendUtil.cpp
clang/test/CodeGenCXX/type-metadata-thinlto.cpp
Index: clang/test/CodeGenCXX/type-metadata-thinlto.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/type-metadata-thinlto.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden
-emit-llvm-bc -o %t %s
+// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s
+
+// CHECK: @_ZTV1A = linkonce_odr
+class A {
+ virtual void f() {}
+};
+
+A *f() {
+ return new A;
+}
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -688,9 +688,11 @@
break;
case Backend_EmitBC:
-PerModulePasses.add(createBitcodeWriterPass(
-*OS, CodeGenOpts.EmitLLVMUseLists, CodeGenOpts.EmitSummaryIndex,
-CodeGenOpts.EmitSummaryIndex));
+if (CodeGenOpts.EmitSummaryIndex)
+ PerModulePasses.add(createWriteThinLTOBitcodePass(*OS));
+else
+ PerModulePasses.add(
+ createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists));
break;
case Backend_EmitLL:
Index: clang/test/CodeGenCXX/type-metadata-thinlto.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/type-metadata-thinlto.cpp
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s
+// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s
+
+// CHECK: @_ZTV1A = linkonce_odr
+class A {
+ virtual void f() {}
+};
+
+A *f() {
+ return new A;
+}
Index: clang/lib/CodeGen/BackendUtil.cpp
===
--- clang/lib/CodeGen/BackendUtil.cpp
+++ clang/lib/CodeGen/BackendUtil.cpp
@@ -688,9 +688,11 @@
break;
case Backend_EmitBC:
-PerModulePasses.add(createBitcodeWriterPass(
-*OS, CodeGenOpts.EmitLLVMUseLists, CodeGenOpts.EmitSummaryIndex,
-CodeGenOpts.EmitSummaryIndex));
+if (CodeGenOpts.EmitSummaryIndex)
+ PerModulePasses.add(createWriteThinLTOBitcodePass(*OS));
+else
+ PerModulePasses.add(
+ createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists));
break;
case Backend_EmitLL:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
