Emit static constexpr member as available_externally definition

[mehdi_amini]

Summary

This enables better optimization, I don't if it is legal c++11 though.

[arphaman]

Does this apply to all constexpr global variables? It could potentially fix https://bugs.llvm.org/show_bug.cgi?id=31860 .

[mehdi_amini]

clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp

12 ↗

(On Diff #105203)

Note: I'm not sure if the standard allows us to assume that we can fold the value for BAR2 without seeing the definition here?
(so we could be even more aggressive)

15 ↗

(On Diff #105203)

Looks like I have a bug here, this should be an internal.

[ahatanak]

clang/lib/CodeGen/CodeGenModule.cpp

2376 ↗

(On Diff #105203)

Does getAnyInitializer ever return a null pointer here when D is a c++11 constexpr?

[mehdi_amini]

clang/lib/CodeGen/CodeGenModule.cpp

2376 ↗

(On Diff #105203)

That's a good question, I wouldn't expect so. I can try adding an assertion instead.
I guess @rsmith could confirm as well.

[rsmith]

We've had problems in the past with speculative emission of values like this resulting in us producing invalid symbol references. (Two cases I remember: an available_externally symbol references a symbol that should be emitted as linkonce_odr but which is not emitted in this TU, and an available_externally symbol references a symbol with hidden visibility that is actually defined in a different DSO. In both cases, if the value of the available_externally symbol is actually used, you end up with a program with an invalid symbol reference.)

I don't immediately see any ways that this change would be susceptible to such a problem, so perhaps our best bet is to try it and see if it actually breaks anything.

clang/lib/CodeGen/CodeGenModule.cpp

2374 ↗

(On Diff #105203)

Applying this for only constexpr variables seems unnecessarily conservative; it seems we could equally do this for any variable that is const and has no mutable fields (though we'd need to check for EmitConstantInit failing in the general case).

2376 ↗

(On Diff #105203)

Only in error cases, which shouldn't get this far.

clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp

1 ↗

(On Diff #105203)

Please also test what happens in C++1z mode, particularly as static constexpr data members are implicitly inline there.

15 ↗

(On Diff #105203)

I would imagine that we skip promotion of declaration to definition in this case if we already have a definition.

To that end, please add a testcase like this:

struct Bar {
  static constexpr int BAZ = 42;
};
auto *use = &Bar::BAZ;
const int Bar::BAZ;

... to make sure that we convert the definition of Bar::BAZ from available_externally to a strong definition (I think we should end up with weak_odr here).

[mehdi_amini]

clang/lib/CodeGen/CodeGenModule.cpp

2374 ↗

(On Diff #105203)

OK I will improve, and include a test with a struct with a mutable field.

clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp

1 ↗

(On Diff #105203)

This is already covered by `clang/test/CodeGenCXX/cxx1z-inline-variables.cpp ` (I actually broke this test during development because I didn't know about inline variable). But I can also add coverage for it here if you'd like.

15 ↗

(On Diff #105203)

weak_odr in C++17 because it is an inline variable, but I expect a strong definition in c++11.
I'll add this, this is a good test-case!

[rsmith]

clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp

15 ↗

(On Diff #105203)

Well, weak_odr is a kind of strong definition :)

Ah, I'd intended to change from emitting this as a strong definition to emitting it as weak_odr even in C++11 (for better forward-compatibility with C++17), but it looks like I never did so. So yes, we'd expect an ExternalLinkage global here (for now) in C++11.

[mehdi_amini]

Fix issues around mutable fields and regression on "internal", add more testing.

[mehdi_amini]

@rsmith: post-C++-commitee-meeting ping ;)

[mehdi_amini]

Weekly ping! (@rsmith)

[mehdi_amini]

Ping again @rsmith (or anyone else)

[rsmith]

clang/lib/CodeGen/CodeGenModule.cpp

2293 ↗

(On Diff #105787)

This comment change suggests that when ExcludeCtor is true, a trivial constructor is considered. That's the opposite of what this change does -- namely, a trivial constructor is not considered regardless of the value of ExcludeCtor.

2302 ↗

(On Diff #105787)

This change looks wrong: you don't know that the default constructor would be used to initialize the object in question, so whether the default constructor is trivial seems like it should have no bearing on the result.

I think you can change the code below that uses GV->isConstant() to call isTypeConstant instead, and remove the need for any changes here.

2433 ↗

(On Diff #105787)

Typo "it's" -> "its"

2435 ↗

(On Diff #105787)

Instead of GV->isConstant(), it would make more sense to use isTypeConstant(D->getType(), true) here.

2437–2445 ↗

(On Diff #105787)

This duplicates much of the work done by isTypeConstant.

2448–2451 ↗

(On Diff #105787)

In order for this transformation to be correct, you need to know that the variable has static initialization, which means that it needs to formally have a constant initializer. You can use D->isInitKnownICE() to check this.

[mehdi_amini]

Address Richard's comment

[mehdi_amini]

clang/lib/CodeGen/CodeGenModule.cpp

2293 ↗

(On Diff #105787)

Yep your right. I removed all the changes here.

2437–2445 ↗

(On Diff #105787)

Indeed, thanks for pointing this, this simplifies a lot!

2448–2451 ↗

(On Diff #105787)

Done! But couldn't find how to express it as a test-case though.

[mehdi_amini]

Bi-weekly ping! (@rsmith)

[arphaman]

FYI, this doesn't compiler after John's constant emitter refactoring (r310964)

[rsmith]

I'd like to also see a testcase for the situation where we trigger the emission of a declaration with an available_externally definition and then find we need to promote it to a "full" definition:

struct A {
  static const int Foo = 123;
};
int *p = &A::Foo; // emit available_externally
const int A::Foo; // convert to full definition

clang/lib/CodeGen/CodeGenModule.cpp

2445 ↗

(On Diff #109694)

Do we need to special-case this? Such declarations are definitions.

2448–2451 ↗

(On Diff #105787)

You'd need to use an initializer that we can constant-fold, such as:

struct A {
  static const int n;
};
bool check() {
  assert(A::n == 0 && "already initialized!");
  return true;
}
const int A::n = (check() || true) ? 1 : 2;

[mehdi_amini]

I'd like to also see a testcase for the situation where we trigger the emission of a declaration with an available_externally definition and then find we need to promote it to a "full" definition:

Added!

clang/lib/CodeGen/CodeGenModule.cpp

2445 ↗

(On Diff #109694)

We don't! Simplified after rebasing and adapting after John's changes.

2448–2451 ↗

(On Diff #105787)

But this wouldn't be an "available_externally". We need the initializer in the class definition.

I tried multiple variant but couldn't get one that compile:

struct B {
  static bool check() {
    if (B::n == 0) return false;
    return true;
  }
  // error: in-class initializer for static data member is not a constant expression
  static const int n = (B::check() || true) ? 1 : 2;
};

struct B {
  static constexpr bool check() {
// "error: constexpr function never produces a constant expression"
    return  (B::n == 0 || true) ? false : true;
  }
//"note: initializer of 'n' is not a constant expression"
  static constexpr int n = (B::check() || true) ? 1 : 2;
};

cfe/trunk/lib/CodeGen/CodeGenModule.cpp

if (D) {
     }
 
     // Handle XCore specific ABI requirements.
     if (getTriple().getArch() == llvm::Triple::xcore &&
         D->getLanguageLinkage() == CLanguageLinkage &&
         D->getType().isConstant(Context) &&
         isExternallyVisible(D->getLinkageAndVisibility().getLinkage()))
       GV->setSection(".cp.rodata");
+
+    // Check if we a have a const declaration with an initializer, we may be
+    // able to emit it as available_externally to expose it's value to the
+    // optimizer.
+    if (Context.getLangOpts().CPlusPlus && GV->hasExternalLinkage() &&
+        D->getType().isConstQualified() && !GV->hasInitializer() &&
+        !D->hasDefinition() && D->hasInit() && !D->hasAttr<DLLImportAttr>()) {
+      const auto *Record =
+          Context.getBaseElementType(D->getType())->getAsCXXRecordDecl();
+      bool HasMutableFields = Record && Record->hasMutableFields();
+      if (!HasMutableFields) {
+        const VarDecl *InitDecl;
+        const Expr *InitExpr = D->getAnyInitializer(InitDecl);
+        if (InitExpr) {
+          GV->setConstant(true);
+          GV->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
+          ConstantEmitter emitter(*this);
+          GV->setInitializer(emitter.tryEmitForInitializer(*InitDecl));
+          emitter.finalize(GV);
+        }
+      }
+    }
   }
 
   auto ExpectedAS =
       D ? D->getType().getAddressSpace()
         : static_cast<unsigned>(LangOpts.OpenCL ? LangAS::opencl_global
                                                 : LangAS::Default);
   assert(getContext().getTargetAddressSpace(ExpectedAS) ==
          Ty->getPointerAddressSpace());

cfe/trunk/test/CodeGenCXX/cxx11-extern-constexpr.cpp

+// RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK --check-prefix=CXX11
+// RUN: %clang_cc1 -std=c++1z %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK --check-prefix=CXX17
+
+struct A {
+  static const int Foo = 123;
+};
+// CHECK: @_ZN1A3FooE = constant i32 123, align 4
+const int *p = &A::Foo; // emit available_externally
+const int A::Foo;       // convert to full definition
+
+struct Bar {
+  int b;
+};
+
+struct MutableBar {
+  mutable int b;
+};
+
+struct Foo {
+  // CXX11: @_ZN3Foo21ConstexprStaticMemberE = available_externally constant i32 42,
+  // CXX17: @_ZN3Foo21ConstexprStaticMemberE = linkonce_odr constant i32 42,
+  static constexpr int ConstexprStaticMember = 42;
+  // CHECK: @_ZN3Foo17ConstStaticMemberE = available_externally constant i32 43,
+  static const int ConstStaticMember = 43;
+
+  // CXX11: @_ZN3Foo23ConstStaticStructMemberE = available_externally constant %struct.Bar { i32 44 },
+  // CXX17: @_ZN3Foo23ConstStaticStructMemberE = linkonce_odr constant %struct.Bar { i32 44 },
+  static constexpr Bar ConstStaticStructMember = {44};
+
+  // CXX11: @_ZN3Foo34ConstexprStaticMutableStructMemberE = external global %struct.MutableBar,
+  // CXX17: @_ZN3Foo34ConstexprStaticMutableStructMemberE = linkonce_odr global %struct.MutableBar { i32 45 },
+  static constexpr MutableBar ConstexprStaticMutableStructMember = {45};
+};
+// CHECK: @_ZL15ConstStaticexpr = internal constant i32 46,
+static constexpr int ConstStaticexpr = 46;
+// CHECK: @_ZL9ConstExpr = internal constant i32 46, align 4
+static const int ConstExpr = 46;
+
+// CHECK: @_ZL21ConstexprStaticStruct = internal constant %struct.Bar { i32 47 },
+static constexpr Bar ConstexprStaticStruct = {47};
+
+// CHECK: @_ZL28ConstexprStaticMutableStruct = internal global %struct.MutableBar { i32 48 },
+static constexpr MutableBar ConstexprStaticMutableStruct = {48};
+
+void use(const int &);
+void foo() {
+  use(Foo::ConstexprStaticMember);
+  use(Foo::ConstStaticMember);
+  use(Foo::ConstStaticStructMember.b);
+  use(Foo::ConstexprStaticMutableStructMember.b);
+  use(ConstStaticexpr);
+  use(ConstExpr);
+  use(ConstexprStaticStruct.b);
+  use(ConstexprStaticMutableStruct.b);
+}