r332839 - [CodeGen] Disable aggressive structor optimizations at -O0, take 2
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Tue May 29 17:49:37 PDT 2018
Sorry, this is still resulting in problems: we're seeing miscompiles and
"definition with same mangled name as another definition" errors after this
change. It seems plausible that this is a pre-existing issue that's just
being exposed by this change, but either way I've temporarily reverted this
in r333482. I'll get you more details, including steps to reproduce this,
offline.
On 21 May 2018 at 04:47, Pavel Labath via cfe-commits <
cfe-commits at lists.llvm.org> wrote:
> Author: labath
> Date: Mon May 21 04:47:45 2018
> New Revision: 332839
>
> URL: http://llvm.org/viewvc/llvm-project?rev=332839&view=rev
> Log:
> [CodeGen] Disable aggressive structor optimizations at -O0, take 2
>
> The first version of the patch (r332228) was flawed because it was
> putting structors into C5/D5 comdats very eagerly. This is correct only
> if we can ensure the comdat contains all required versions of the
> structor (which wasn't the case). This version uses a more nuanced
> approach:
> - for local structor symbols we use an alias because we don't have to
> worry about comdats or other compilation units.
> - linkonce symbols are emitted separately, as we cannot guarantee we
> will have all symbols we need to form a comdat (they are emitted
> lazily, only when referenced).
> - available_externally symbols are also emitted separately, 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).
>
> Reviewers: rjmccall, aprantl
>
> Subscribers: cfe-commits
>
> Differential Revision: https://reviews.llvm.org/D46685
>
> Modified:
> cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
> cfe/trunk/test/CodeGenCXX/ctor-dtor-alias.cpp
> cfe/trunk/test/CodeGenCXX/float16-declarations.cpp
>
> Modified: cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/
> ItaniumCXXABI.cpp?rev=332839&r1=332838&r2=332839&view=diff
> ============================================================
> ==================
> --- cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp (original)
> +++ cfe/trunk/lib/CodeGen/ItaniumCXXABI.cpp Mon May 21 04:47:45 2018
> @@ -3628,12 +3628,22 @@ static StructorCodegen getCodegenToUse(C
> }
> 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).
>
> Modified: cfe/trunk/test/CodeGenCXX/ctor-dtor-alias.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/
> CodeGenCXX/ctor-dtor-alias.cpp?rev=332839&r1=332838&r2=332839&view=diff
> ============================================================
> ==================
> --- cfe/trunk/test/CodeGenCXX/ctor-dtor-alias.cpp (original)
> +++ cfe/trunk/test/CodeGenCXX/ctor-dtor-alias.cpp Mon May 21 04:47:45 2018
> @@ -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 @@ namespace test1 {
> // 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 @@ template struct foobar<void>;
> }
>
> 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 @@ namespace test3 {
> // 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 @@ namespace test4 {
> // 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 @@ namespace test7 {
> // 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() {}
> };
>
> Modified: cfe/trunk/test/CodeGenCXX/float16-declarations.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/
> CodeGenCXX/float16-declarations.cpp?rev=332839&
> r1=332838&r2=332839&view=diff
> ============================================================
> ==================
> --- cfe/trunk/test/CodeGenCXX/float16-declarations.cpp (original)
> +++ cfe/trunk/test/CodeGenCXX/float16-declarations.cpp Mon May 21
> 04:47:45 2018
> @@ -103,7 +103,7 @@ int main(void) {
>
> 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
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180529/2d86d1fc/attachment-0001.html>
More information about the cfe-commits
mailing list