labath updated this revision to Diff 147315.
labath added a comment.
Updating with a new version of the patch. The problem with the previous version
was that it caused us to use C5/https://reviews.llvm.org/D5 comdats very
aggressively (at -O0), and this
is not always correct. This uses a more nuanced approach:
- for local symbols we can use aliases because we don't have to worry about
comdats or other compilation units.
- for linkonce symbols, we must emit each structor separately. Theoretically it
would be possible to emit a C5 comdat, but then we would need to make sure it
contains both (in case of destructors, all three) symbols. As linkonce symbols
are emitted lazily, I did not find an easy way to ensure that this is always
the case.
- for available_externally, I also emit the structor, as the code seemed to be
worried about emitting an alias in this case.
- other linkage types are not affected by the optimization level. They either
get put into a comdat (weak) or get aliased (external).
I also add more thorough tests for this behavior.
Overall I am not sure if this extra complexity is worth the few extra aliases we
can emit this way at -O0. Let me know what you think.
Repository:
rC Clang
https://reviews.llvm.org/D46685
Files:
lib/CodeGen/ItaniumCXXABI.cpp
test/CodeGenCXX/ctor-dtor-alias.cpp
test/CodeGenCXX/float16-declarations.cpp
Index: test/CodeGenCXX/float16-declarations.cpp
===================================================================
--- test/CodeGenCXX/float16-declarations.cpp
+++ test/CodeGenCXX/float16-declarations.cpp
@@ -103,7 +103,7 @@
C1 c1(f1l);
// CHECK-DAG: [[F1L:%[a-z0-9]+]] = load half, half* %{{.*}}, align 2
-// CHECK-DAG: call void @_ZN2C1C2EDF16_(%class.C1* %{{.*}}, half %{{.*}})
+// CHECK-DAG: call void @_ZN2C1C1EDF16_(%class.C1* %{{.*}}, half %{{.*}})
S1<_Float16> s1 = { 132.f16 };
// CHECK-DAG: @_ZZ4mainE2s1 = private unnamed_addr constant %struct.S1 { half 0xH5820 }, align 2
Index: test/CodeGenCXX/ctor-dtor-alias.cpp
===================================================================
--- test/CodeGenCXX/ctor-dtor-alias.cpp
+++ test/CodeGenCXX/ctor-dtor-alias.cpp
@@ -1,5 +1,7 @@
-// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases | FileCheck --check-prefix=NOOPT %s
-
+// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases > %t
+// RUN: FileCheck --check-prefix=NOOPT1 --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT2 --input-file=%t %s
+// RUN: FileCheck --check-prefix=NOOPT3 --input-file=%t %s
// RUN: %clang_cc1 %s -triple i686-linux -emit-llvm -o - -mconstructor-aliases -O1 -disable-llvm-passes > %t
// RUN: FileCheck --check-prefix=CHECK1 --input-file=%t %s
// RUN: FileCheck --check-prefix=CHECK2 --input-file=%t %s
@@ -21,6 +23,13 @@
// CHECK1: define weak_odr void @_ZN5test16foobarIvED0Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
// CHECK1-NOT: comdat
+// This should happen regardless of the opt level.
+// NOOPT1: @_ZN5test16foobarIvEC1Ev = weak_odr alias void {{.*}} @_ZN5test16foobarIvEC2Ev
+// NOOPT1: @_ZN5test16foobarIvED1Ev = weak_odr alias void (%"struct.test1::foobar"*), void (%"struct.test1::foobar"*)* @_ZN5test16foobarIvED2Ev
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvEC2Ev({{.*}} comdat($_ZN5test16foobarIvEC5Ev)
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvED2Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
+// NOOPT1: define weak_odr void @_ZN5test16foobarIvED0Ev({{.*}} comdat($_ZN5test16foobarIvED5Ev)
+
// COFF doesn't support comdats with arbitrary names (C5/D5).
// COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvEC2Ev({{.*}} comdat align
// COFF: define weak_odr {{.*}} void @_ZN5test16foobarIvEC1Ev({{.*}} comdat align
@@ -37,12 +46,17 @@
}
namespace test2 {
-// test that when the destrucor is linkonce_odr we just replace every use of
+// test that when the destructor is linkonce_odr we just replace every use of
// C1 with C2.
// CHECK1: define internal void @__cxx_global_var_init()
// CHECK1: call void @_ZN5test26foobarIvEC2Ev
// CHECK1: define linkonce_odr void @_ZN5test26foobarIvEC2Ev({{.*}} comdat align
+
+// At -O0, we should still emit the complete constructor.
+// NOOPT1: define internal void @__cxx_global_var_init()
+// NOOPT1: call void @_ZN5test26foobarIvEC1Ev
+// NOOPT1: define linkonce_odr void @_ZN5test26foobarIvEC1Ev({{.*}} comdat align
void g();
template <typename T> struct foobar {
foobar() { g(); }
@@ -57,6 +71,11 @@
// CHECK1: define internal void @__cxx_global_var_init.1()
// CHECK1: call i32 @__cxa_atexit{{.*}}_ZN5test312_GLOBAL__N_11AD2Ev
// CHECK1: define internal void @_ZN5test312_GLOBAL__N_11AD2Ev(
+
+// We can use an alias for internal symbols at -O0.
+// NOOPT2: _ZN5test312_GLOBAL__N_11BD1Ev = internal alias void {{.*}} @_ZN5test312_GLOBAL__N_11BD2Ev
+// NOOPT2: define internal void @__cxx_global_var_init.1()
+// NOOPT2: call i32 @__cxa_atexit{{.*}}_ZN5test312_GLOBAL__N_11BD1Ev
namespace {
struct A {
~A() {}
@@ -77,11 +96,12 @@
// CHECK1: call i32 @__cxa_atexit{{.*}}_ZN5test41AD2Ev
// CHECK1: define linkonce_odr void @_ZN5test41AD2Ev({{.*}} comdat align
- // test that we don't do this optimization at -O0 so that the debugger can
- // see both destructors.
- // NOOPT: define internal void @__cxx_global_var_init.2()
- // NOOPT: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD2Ev
- // NOOPT: define linkonce_odr void @_ZN5test41BD2Ev({{.*}} comdat align
+ // Test that we don't do this optimization at -O0 and call the complete
+ // destructor for B instead. This enables the debugger to see both
+ // destructors.
+ // NOOPT2: define internal void @__cxx_global_var_init.2()
+ // NOOPT2: call i32 @__cxa_atexit{{.*}}@_ZN5test41BD1Ev
+ // NOOPT2: define linkonce_odr void @_ZN5test41BD1Ev({{.*}} comdat align
struct A {
virtual ~A() {}
};
@@ -129,6 +149,11 @@
// out if we should).
// pr17875.
// CHECK3: define void @_ZN5test71BD2Ev
+
+ // At -O0, we should emit both destructors, the complete can be an alias to
+ // the base one.
+ // NOOPT3: @_ZN5test71BD1Ev = alias void {{.*}} @_ZN5test71BD2Ev
+ // NOOPT3: define void @_ZN5test71BD2Ev
template <typename> struct A {
~A() {}
};
Index: lib/CodeGen/ItaniumCXXABI.cpp
===================================================================
--- lib/CodeGen/ItaniumCXXABI.cpp
+++ lib/CodeGen/ItaniumCXXABI.cpp
@@ -3630,12 +3630,22 @@
}
llvm::GlobalValue::LinkageTypes Linkage = CGM.getFunctionLinkage(AliasDecl);
- if (llvm::GlobalValue::isDiscardableIfUnused(Linkage))
- return StructorCodegen::RAUW;
-
+ // All discardable structors can be RAUWed, but we don't want to do that in
+ // unoptimized code, as that makes complete structor symbol disappear
+ // completely, which degrades debugging experience.
+ // Symbols with private linkage can be safely aliased, so we special case them
+ // here.
+ if (llvm::GlobalValue::isLocalLinkage(Linkage))
+ return CGM.getCodeGenOpts().OptimizationLevel > 0 ? StructorCodegen::RAUW
+ : StructorCodegen::Alias;
+
+ // Linkonce structors cannot be aliased nor placed in a comdat, so these need
+ // to be emitted separately.
// FIXME: Should we allow available_externally aliases?
- if (!llvm::GlobalAlias::isValidLinkage(Linkage))
- return StructorCodegen::RAUW;
+ if (llvm::GlobalValue::isDiscardableIfUnused(Linkage) ||
+ !llvm::GlobalAlias::isValidLinkage(Linkage))
+ return CGM.getCodeGenOpts().OptimizationLevel > 0 ? StructorCodegen::RAUW
+ : StructorCodegen::Emit;
if (llvm::GlobalValue::isWeakForLinker(Linkage)) {
// Only ELF and wasm support COMDATs with arbitrary names (C5/D5).
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
