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

13

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)

16

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

[ahatanak]

clang/lib/CodeGen/CodeGenModule.cpp

2375

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

[mehdi_amini]

clang/lib/CodeGen/CodeGenModule.cpp

2375

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

2373

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).

2375

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

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

2

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

16

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

2373

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

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

2

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.

16

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

16

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

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

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

Typo "it's" -> "its"

2435

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

2437–2445

This duplicates much of the work done by isTypeConstant.

2448–2451

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

Yep your right. I removed all the changes here.

2437–2445

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

2448–2451

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

2437

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

2448–2451

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

2437

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

2448–2451

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;
};

clang/lib/CodeGen/CodeGenModule.cpp

if (F->empty())
       F->setCallingConv(getBuiltinCC());
   return C;
 }
 
 /// isTypeConstant - Determine whether an object of this type can be emitted
 /// as a constant.
 ///
 /// If ExcludeCtor is true, the duration when the object's constructor runs
-/// will not be considered. The caller will need to verify that the object is
-/// not written to during its construction.
+/// will not be considered (unless trivial). The caller will need to verify that
+/// the object is not written to during its construction.
 bool CodeGenModule::isTypeConstant(QualType Ty, bool ExcludeCtor) {
   if (!Ty.isConstant(Context) && !Ty->isReferenceType())
     return false;
 
   if (Context.getLangOpts().CPlusPlus) {
-    if (const CXXRecordDecl *Record
-          = Context.getBaseElementType(Ty)->getAsCXXRecordDecl())
-      return ExcludeCtor && !Record->hasMutableFields() &&
-             Record->hasTrivialDestructor();
+    if (const CXXRecordDecl *Record =
+            Context.getBaseElementType(Ty)->getAsCXXRecordDecl())
+      return (ExcludeCtor || Record->hasTrivialDefaultConstructor()) &&
+             !Record->hasMutableFields() && Record->hasTrivialDestructor();
   }
 
   return true;
 }
 
 /// GetOrCreateLLVMGlobal - If the specified mangled name is not in the module,
 /// create and return an llvm GlobalVariable with the specified type.  If there
 /// is something in the module with the specified name, return it potentially
if (Entry) {
     // global, not just return a bitcast.)
     if (!IsForDefinition)
       return llvm::ConstantExpr::getBitCast(Entry, Ty);
   }
 
   auto AddrSpace = GetGlobalVarAddressSpace(D);
   auto TargetAddrSpace = getContext().getTargetAddressSpace(AddrSpace);
 
   auto *GV = new llvm::GlobalVariable(
       getModule(), Ty->getElementType(), false,
-      llvm::GlobalValue::ExternalLinkage, nullptr, MangledName, nullptr,
+      llvm::GlobalVariable::ExternalLinkage, nullptr, MangledName, nullptr,
       llvm::GlobalVariable::NotThreadLocal, TargetAddrSpace);
 
   // If we already created a global with the same mangled name (but different
   // type) before, take its name and remove it from its parent.
   if (Entry) {
     GV->takeName(Entry);
 
     if (!Entry->use_empty()) {
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 (GV->hasExternalLinkage() && GV->isConstant() && !GV->hasInitializer() &&
+        !D->hasDefinition() && D->hasInit()) {
+      bool HasMutableField = false;
+      if (Context.getLangOpts().CPlusPlus) {
+        if (const CXXRecordDecl *Record =
+                Context.getBaseElementType(D->getType())->getAsCXXRecordDecl())
+          HasMutableField = Record->hasMutableFields();
+      }
+
+      if (D->getType().isConstQualified() &&
+          !HasMutableField & !D->isInline() && !D->hasAttr<DLLImportAttr>()) {
+        const VarDecl *InitDecl;
+        const Expr *InitExpr = D->getAnyInitializer(InitDecl);
+        if (InitExpr) {
+          GV->setLinkage(llvm::GlobalValue::AvailableExternallyLinkage);
+          GV->setInitializer(EmitConstantInit(*InitDecl));
+        }
+      }
+    }
   }
 
   auto ExpectedAS =
       D ? D->getType().getAddressSpace()
         : static_cast<unsigned>(LangOpts.OpenCL ? LangAS::opencl_global
                                                 : LangAS::Default);
   assert(getContext().getTargetAddressSpace(ExpectedAS) ==
          Ty->getPointerAddressSpace());

clang/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 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);
+}