CodeGen: Emit globals with a constant initializer but no definition as available_externally.

[bkramer]

Summary

This can happen with static const members (PR17612). It has the advantage
over just making constant folding smarter that it also enables the optimizer
to fold more complicated patterns, such as passing the static const member
by reference.

We only do this at -O1 or higher.

[bkramer]

Fixed a crasher during selfhost and tightened up asserts.

[rnk]

lib/CodeGen/CodeGenModule.cpp

1279–1282

This seems more intuitive as:

if (isAvailableExternally)
  return CGO.OptLevel != 0;
return true;

Also, how do we know isConstant is true? Can you assert that it is like you do below?

1884

What if the variable is thread local, or fits any of the below special cases involving templates? This probably goes towards the end of the if/else chain, not first, considering that it's for an optimization.

[bkramer]

lib/CodeGen/CodeGenModule.cpp

1279–1282

Will fix the order. The isConstant flag has no effect for this path currently, but I can add the assert here too.

1884

The problem is that we now allow certain types of globals to hit the method that didn't get here before, such as explicit template instantiations. For example std::string is explicitly instantiated in libc++, but std::string::npos is still a good fit for this optimization. GetGVALinkageForVariable asserts in that case, so the options are

• replicating the checks from GetGVALinkageForVariable here.
• leaving the check where it is. I'm not aware of any valid C++11 fragment that this code would fail for, but that's of course a weak argument when C++ is moving on :/

Any preferences?

[bkramer]

Add some clarification and address review comments.

[rnk]

lib/CodeGen/CodeGenModule.cpp

1884

I guess there's no GVALinkage value for available_externally and we probably don't want to add one, so it might make sense to me to keep this here.

Can you add a test case for something like std::string::npos where it should be considered available_externally?

I can also come up with weird test cases along the lines of:

constexpr int foo() { return 1; }
struct Holder {
  static const int thread_local f = foo();
};
const int *bar() {
  return &Holder::f;
}

I'm not sure what the linkage of 'f' should actually be, but having this as a test case would be good. :)

[bkramer]

Extended test case for explicit instantiations and thread_locals.

[bkramer]

Ping.

[rsmith]

Seems like a great idea to me.

lib/CodeGen/CodeGenModule.cpp

1591

This is incorrect. The following is valid:

struct S {
  static constexpr volatile int n = 0;
};

1884–1885

This is incorrect. The following is valid:

struct A {
  mutable int n;
};
struct B {
  static constexpr A a = { 0 };
};

[bkramer]

Turned assertions into checks.

[bkramer]

Ping. The patch is old but still useful :)

[• rafael]

The final LGTM should probably wait for a C++ expert, but I agree this would be really nice to have :-)

test/CodeGenCXX/constexpr-available-externally.cpp

6

Can you move this closer to the code that causes it to be output? Maybe with CHECK-DAG?

37

Could you also add a testcase with a explicit template instantiation declaration?

Please also add a test showing we don't emit a available_externally for a member if just the class is used:

class foo {

static const int X = 1;
void bar();

}
void f(foo *x) {x->bar(); }

LGTM with that.

[bkramer]

Update patch. Add a test that we don't emit unused variables.

[rsmith]

I don't like that parts of the test here are duplicated in three different places; can you refactor to improve that? Maybe factor out a CanEmitAsAvailableExternally from GetOrCreateLLVMGlobal and GetLLVMLinkageVarDefinition, call isTypeConstant in there rather than asserting it, and remove the assert from shouldEmitGlobalVariable?

It also seems weird to me that shouldEmitGlobalVariable would return true when GetLLVMLinkageVarDefinition returns llvm::GlobalVariable::ExternalLinkage (even though in practice this never actually happens).

lib/CodeGen/CodeGenModule.cpp

1884–1885

Why is this an assert rather than a test?

[bkramer]

Factored duplicated checks into a helper method.

[bkramer]

lib/CodeGen/CodeGenModule.cpp

1884–1885

The idea was that this code path could never be triggered if the check failed earlier. But that's obsolete with the latest update.

[rsmith]

Patch looks good to me. Just one more thing (hopefully...): does this behave correctly if the global variable lifetime-extends a temporary? Given:

struct Q { int &&r; };
struct S {
  static constexpr Q q = { 0 };
};
int k = S::q.r;

... how do we emit @_ZN1S1qE and @_ZGRN1S1qE?

lib/CodeGen/CodeGenModule.cpp

1284–1289

Can you find a better name for this function? It returns true for variables that we shouldn't emit at all, which makes me uneasy. Maybe just inline it into its only call site (which already has a comment describing exactly what this code does)?

[bkramer]

@_ZN1S1qE = available_externally constant %struct.Q { i32* @_ZGRN1S1qE }, align 8
@_ZGRN1S1qE = private global i32 0, align 4

That doesn't look good :|

[rsmith]

This seems to be a pre-existing and essentially unrelated bug. The same problem occurs for:

constexpr int &p = S::q.r;

I think we should be looking at the linkage of the extending declaration when picking the linkage for a lifetime-extended temporary. (If the extending declaration is externally visible, I think the temporary should be linkonce_odr.)

[sepavloff]

{F51457}
Could this small change be useful for solving the problem?

[rsmith]

I don't think the available_externally check is right, but that looks like the right direction.

lib/CodeGen/CodeGenModule.h

class CodeGenModule : public CodeGenTypeCache {
   /// Lazily create the Objective-C runtime
   void createObjCRuntime();
 
   void createOpenCLRuntime();
   void createCUDARuntime();
 
   bool isTriviallyRecursive(const FunctionDecl *F);
   bool shouldEmitFunction(GlobalDecl GD);
+  bool shouldEmitGlobalVariable(const VarDecl *D);
 
   /// @name Cache for Blocks Runtime Globals
   /// @{
 
   llvm::Constant *NSConcreteGlobalBlock;
   llvm::Constant *NSConcreteStackBlock;
 
   llvm::Constant *BlockObjectAssign;

lib/CodeGen/CodeGenModule.cpp

CodeGenModule::shouldEmitFunction(GlobalDecl GD) {
   // PR9614. Avoid cases where the source code is lying to us. An available
   // externally function should have an equivalent function somewhere else,
   // but a function that calls itself is clearly not equivalent to the real
   // implementation.
   // This happens in glibc's btowc and in some configure checks.
   return !isTriviallyRecursive(F);
 }
 
+bool CodeGenModule::shouldEmitGlobalVariable(const VarDecl *VD) {
+  if (GetLLVMLinkageVarDefinition(VD, /*isConstant=*/true) ==
+      llvm::Function::AvailableExternallyLinkage) {
+    assert(isTypeConstant(D->getType(), /*ExcludeCtor=*/true) &&
+           "available_externally variable should be constant!");
+    return CodeGenOpts.OptimizationLevel != 0;
+  }
+  return true;
+}
+
 /// If the type for the method's class was generated by
 /// CGDebugInfo::createContextChain(), the cache contains only a
 /// limited DIType without any declarations. Since EmitFunctionStart()
 /// needs to find the canonical declaration for each method, we need
 /// to construct the complete type prior to emitting the method.
 void CodeGenModule::CompleteDIClassType(const CXXMethodDecl* D) {
   if (!D->isInstance())
     return;
 
   if (CGDebugInfo *DI = getModuleDebugInfo())
if (const CXXMethodDecl *Method = dyn_cast<CXXMethodDecl>(D)) {
         getVTables().EmitThunks(GD);
 
       return;
     }
 
     return EmitGlobalFunctionDefinition(GD);
   }
   
-  if (const VarDecl *VD = dyn_cast<VarDecl>(D))
+  if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
+    // At -O0, don't generate IR for vars with available_externally linkage.
+    if (!shouldEmitGlobalVariable(VD))
+      return;
+
     return EmitGlobalVarDefinition(VD);
+  }
   
   llvm_unreachable("Invalid argument to EmitGlobalDefinition()");
 }
 
 /// GetOrCreateLLVMFunction - If the specified mangled name is not in the
 /// module, create and return an llvm Function with the specified type. If there
 /// is something in the module with the specified name, return it potentially
 /// bitcasted to the right type.
if (LV.getLinkage() != ExternalLinkage) {
         GV->setVisibility(GetLLVMVisibility(LV.getVisibility()));
     }
 
     if (D->getTLSKind()) {
       if (D->getTLSKind() == VarDecl::TLS_Dynamic)
         CXXThreadLocals.push_back(std::make_pair(D, GV));
       setTLSMode(GV, *D);
     }
+
+    // If the variable has an initializer but no definition we can emit it with
+    // available_externally linkage.
+    if (D->hasInit() && !D->hasDefinition(Context)) {
+      assert(!D->getType().isVolatileQualified());
+      assert(GetLLVMLinkageVarDefinition(D, /*isConstant=*/true) ==
+                 llvm::GlobalValue::AvailableExternallyLinkage &&
+             "Global variable should be available_externally!");
+      DeferredDeclsToEmit.push_back(D);
+    }
   }
 
   if (AddrSpace != Ty->getAddressSpace())
     return llvm::ConstantExpr::getBitCast(GV, Ty);
   else
     return GV;
 }
 
void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D) {
   // Emit global variable debug information.
   if (CGDebugInfo *DI = getModuleDebugInfo())
     if (getCodeGenOpts().getDebugInfo() >= CodeGenOptions::LimitedDebugInfo)
       DI->EmitGlobalVariable(GV, D);
 }
 
 llvm::GlobalValue::LinkageTypes
 CodeGenModule::GetLLVMLinkageVarDefinition(const VarDecl *D, bool isConstant) {
+  // Check for definitionless vars. We can emit them with available_externally
+  // linkage. We check this before calling GetGVALinkageForVariable because it
+  // can happen for explicit template instantiations that would cause an
+  // assertion failure otherwise.
+  if (D->hasInit() && !D->hasDefinition(Context)) {
+    assert(isTypeConstant(D->getType(), /*ExcludeCtor=*/true) &&
+           "Variable with init but no definition should be constant!");
+    return llvm::GlobalValue::AvailableExternallyLinkage;
+  }
+
   GVALinkage Linkage = getContext().GetGVALinkageForVariable(D);
   if (Linkage == GVA_Internal)
     return llvm::Function::InternalLinkage;
   else if (D->hasAttr<DLLImportAttr>())
     return llvm::Function::DLLImportLinkage;
   else if (D->hasAttr<DLLExportAttr>())
     return llvm::Function::DLLExportLinkage;
   else if (D->hasAttr<SelectAnyAttr>()) {

test/CodeGenCXX/constexpr-available-externally.cpp

+// RUN: %clang_cc1 -emit-llvm -O1 -std=c++11 -disable-llvm-optzns -o - %s | FileCheck %s
+
+// PR17612
+// CHECK: @_ZN6Object3fooE = available_externally constant %struct.Literal { i32 1 }
+// CHECK: @_ZN6Object3barE = available_externally constant i32 42
+
+struct Literal {
+  int i;
+  constexpr Literal(int i) : i(i) {}
+};
+
+struct Object {
+  static constexpr Literal foo = Literal(1);
+  static const int bar = 42;
+};
+
+void bar(const int &);
+
+int f() {
+  Literal l = Object::foo;
+  bar(Object::bar);
+  return l.i;
+
+// CHECK-LABEL: @_Z1fv
+// CHECK: call void @llvm.memcpy{{[^(]*}}(i8* %{{[^,]*}}, i8* bitcast (%struct.Literal* @_ZN6Object3fooE to i8*)
+// CHECK: call void @_Z3barRKi(i32* @_ZN6Object3barE)
+}