Skip to content

Conversation

@Fznamznon
Copy link
Contributor

@Fznamznon Fznamznon commented Sep 4, 2025

Because the initializers for static locals were only emitted once the body
of the parent functions were emitted, we were always missing
initializers for static locals defined inside of consteval functions
because their bodies are never handled by CodeGen. This patch attempts
to fix that by fitting such variables into existing lazy emission
system.

Fixes #82994

Do not wait until the body of the function is met. This helps with
initializing static variables in conseval functions.

Fixes llvm#82994
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. labels Sep 4, 2025
@llvmbot
Copy link
Member

llvmbot commented Sep 4, 2025

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Mariya Podchishchaeva (Fznamznon)

Changes

Do not wait until the body of the function is met. This helps with initializing static variables in conseval functions.

Fixes #82994


Full diff: https://github.com/llvm/llvm-project/pull/156933.diff

4 Files Affected:

  • (modified) clang/lib/CodeGen/CGDecl.cpp (+17-4)
  • (modified) clang/test/CodeGenCXX/pr47636.cpp (+1-1)
  • (modified) clang/test/CodeGenCXX/reference-init.cpp (+1-1)
  • (added) clang/test/CodeGenCXX/static-constexpr-in-consteval.cpp (+73)
diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index 29193e0c541b9..da030f4c73779 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -275,18 +275,31 @@ llvm::Constant *CodeGenModule::getOrCreateStaticVarDecl(
   LangAS AS = GetGlobalVarAddressSpace(&D);
   unsigned TargetAS = getContext().getTargetAddressSpace(AS);
 
+  Expr::EvalResult EvalResult;
+  llvm::Constant *Init = nullptr;
+  std::optional<ConstantEmitter> Emitter;
   // OpenCL variables in local address space and CUDA shared
   // variables cannot have an initializer.
-  llvm::Constant *Init = nullptr;
   if (Ty.getAddressSpace() == LangAS::opencl_local ||
-      D.hasAttr<CUDASharedAttr>() || D.hasAttr<LoaderUninitializedAttr>())
+      D.hasAttr<CUDASharedAttr>() || D.hasAttr<LoaderUninitializedAttr>()) {
     Init = llvm::UndefValue::get(LTy);
-  else
+  } else if (D.isUsableInConstantExpressions(getContext())) {
+    const Expr *InitExpr = D.getInit();
+    assert(InitExpr && "Usable in constant expressions without initializer?");
+    Emitter.emplace(*this);
+    llvm::Constant *Initializer = Emitter->tryEmitForInitializer(D);
+    Init = Initializer;
+  } else {
     Init = EmitNullConstant(Ty);
+  }
 
   llvm::GlobalVariable *GV = new llvm::GlobalVariable(
-      getModule(), LTy, Ty.isConstant(getContext()), Linkage, Init, Name,
+      getModule(), Init->getType(), Ty.isConstant(getContext()), Linkage, Init, Name,
       nullptr, llvm::GlobalVariable::NotThreadLocal, TargetAS);
+
+  if (Emitter)
+    Emitter->finalize(GV);
+
   GV->setAlignment(getContext().getDeclAlign(&D).getAsAlign());
 
   if (supportsCOMDAT() && GV->isWeakForLinker())
diff --git a/clang/test/CodeGenCXX/pr47636.cpp b/clang/test/CodeGenCXX/pr47636.cpp
index 597e94695ca5f..022e085e28845 100644
--- a/clang/test/CodeGenCXX/pr47636.cpp
+++ b/clang/test/CodeGenCXX/pr47636.cpp
@@ -5,8 +5,8 @@ int(&&intu_rvref)[] {1,2,3,4};
 
 void foo() {
   static const int(&&intu_rvref)[] {1,2,3,4};
-  // CHECK: @_ZZ3foovE10intu_rvref = internal constant ptr @_ZGRZ3foovE10intu_rvref_
   // CHECK: @_ZGRZ3foovE10intu_rvref_ = internal constant [4 x i32] [i32 1, i32 2, i32 3, i32 4]
+  // CHECK: @_ZZ3foovE10intu_rvref = internal constant ptr @_ZGRZ3foovE10intu_rvref_
 }
 
 // Example given on review, ensure this doesn't crash as well.
diff --git a/clang/test/CodeGenCXX/reference-init.cpp b/clang/test/CodeGenCXX/reference-init.cpp
index a98d400eb17a2..67a901dfe6fa3 100644
--- a/clang/test/CodeGenCXX/reference-init.cpp
+++ b/clang/test/CodeGenCXX/reference-init.cpp
@@ -4,8 +4,8 @@
 // expected-no-diagnostics
 
 #if __cplusplus >= 201103L
-// CHECK-CXX11: @_ZZ15InitRefWithListvE1r = internal constant ptr @_ZGRZ15InitRefWithListvE1r_
 // CHECK-CXX11: @_ZGRZ15InitRefWithListvE1r_ = internal constant i32 123
+// CHECK-CXX11: @_ZZ15InitRefWithListvE1r = internal constant ptr @_ZGRZ15InitRefWithListvE1r_
 int InitRefWithList() { static const int &r = {123}; return r; }
 #endif
 
diff --git a/clang/test/CodeGenCXX/static-constexpr-in-consteval.cpp b/clang/test/CodeGenCXX/static-constexpr-in-consteval.cpp
new file mode 100644
index 0000000000000..d818f9725f196
--- /dev/null
+++ b/clang/test/CodeGenCXX/static-constexpr-in-consteval.cpp
@@ -0,0 +1,73 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -std=c++23 %s -emit-llvm -o - | FileCheck %s
+
+consteval const int* a() {
+    static constexpr int b = 3;
+    return &b;
+}
+
+consteval const int returns42() {
+    static constexpr long c = 42;
+    return c;
+}
+
+struct S {
+  int a;
+  constexpr S(int _a) : a(_a){};
+};
+
+consteval auto returns48viastruct() {
+    static constexpr S s{ 48 };
+    return s;
+}
+
+consteval int why() {
+  static constexpr int a = 10;
+  {
+  static constexpr int a = 20;
+  return a;
+  }
+
+}
+
+consteval const int* returnsarray() {
+  static constexpr int a[] = {10, 20, 30};
+  return a;
+}
+
+// CHECK: @_ZZ1avE1b = linkonce_odr constant i32 3, comdat, align 4
+// CHECK: @_ZZ12returnsarrayvE1a = linkonce_odr constant [3 x i32] [i32 10, i32 20, i32 30], comdat, align 4
+
+// CHECK: define dso_local noundef i32 @_Z1cv()
+// CHECK-NEXT: entry:
+// CHECK-NEXT:   %0 = load i32, ptr @_ZZ1avE1b, align 4
+// CHECK-NEXT:   ret i32 %0
+// CHECK-NEXT: }
+int c() { return *a(); }
+
+// CHECK: define dso_local noundef i32 @_Z1dv()
+// CHECK-NEXT: entry:
+// CHECK-NEXT:   ret i32 42
+// CHECK-NEXT: }
+int d() { return returns42(); }
+
+int e() { return returns48viastruct().a; }
+// CHECK: define dso_local noundef i32 @_Z1ev()
+// CHECK-NEXT: entry:
+// CHECK-NEXT:   %ref.tmp = alloca %struct.S, align 4
+// CHECK-NEXT:   %0 = getelementptr inbounds nuw %struct.S, ptr %ref.tmp, i32 0, i32 0
+// CHECK-NEXT:   store i32 48, ptr %0, align 4
+// CHECK-NEXT:   ret i32 48
+// CHECK-NEXT: }
+
+int f() { return why(); }
+// CHECK: define dso_local noundef i32 @_Z1fv()
+// CHECK-NEXT: entry:
+// CHECK-NEXT:   ret i32 20
+// CHECK-NEXT: }
+
+int g() { return returnsarray()[2]; }
+// CHECK: define dso_local noundef i32 @_Z1gv()
+// CHECK-NEXT: entry:
+// CHECK-NEXT:   %0 = load i32, ptr getelementptr inbounds (i32, ptr @_ZZ12returnsarrayvE1a, i64 2), align 4
+// CHECK-NEXT:   ret i32 %0
+// CHECK-NEXT: }

@github-actions
Copy link

github-actions bot commented Sep 4, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Contributor

@rjmccall rjmccall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initializers can be self-referential, so doing this eagerly might be problematic.

Why does this help with initializing things in consteval functions?

@Fznamznon
Copy link
Contributor Author

Why does this help with initializing things in consteval functions?

It seems once codegen meets an odr use of the variable, it doesn't emit the initializer, only the global variable itself. Then while emitting some function's body it meets the variable declaration, and if it was a static variable it adds the initializer via AddInitializerToStaticVarDecl. consteval function bodies are never handled by CodeGen, hence missing initializers. This patch makes sure they won't be missing.

Initializers can be self-referential, so doing this eagerly might be problematic.

Even if a variable is usable in constant expressions?

@rjmccall
Copy link
Contributor

rjmccall commented Sep 4, 2025

It seems once codegen meets an odr use of the variable, it doesn't emit the initializer, only the global variable itself. Then while emitting some function's body it meets the variable declaration, and if it was a static variable it adds the initializer via AddInitializerToStaticVarDecl. consteval function bodies are never handled by CodeGen, hence missing initializers. This patch makes sure they won't be missing.

That makes sense. I think the cleaner way of handling it is probably to just fit it into the existing lazy-emission system, DeferredDeclsToEmit. In fact, we should be able to do that for every static local variable with a constant-expression initializer: don't emit them eagerly when we walk the function body, call addDeferredDeclToEmit whenever we ODR-use them for the first time, and then make sure EmitDeferredDecl does the right thing for them.

Even if a variable is usable in constant expressions?

I believe so, yes. Taking the address of an object of static storage duration is generally allowed in constant expressions, and there's no restriction against that being the current declaration or otherwise forming cycles.

Because the initializers for static locals were only emitted once the body
of the parent functions were emitted, we were always missing
initializers for static locals defined inside of consteval functions
because their bodies are never handled by CodeGen. This patch attempts
to fix that by fitting such variables into existing lazy emission
system.
@Fznamznon Fznamznon changed the title [clang] Emit initializers for static const/constexpr variables once they're met [clang] Fix static local variables in consteval functions Sep 10, 2025
@Fznamznon
Copy link
Contributor Author

That makes sense. I think the cleaner way of handling it is probably to just fit it into the existing lazy-emission system, DeferredDeclsToEmit

I've tried to do that. Thanks for the suggestion.
I also added a test with a self referential initializer, which with the previous patch crashed due to endless recursion.

@Fznamznon Fznamznon requested a review from rjmccall September 10, 2025 15:05

setStaticLocalDeclAddress(&D, Addr);
if (HasConstantInit && D.isStaticLocal())
addDeferredDeclToEmit(GlobalDecl(&D));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is D.isStaticLocal() ever actually false here? I'm worried about this condition potentially getting out of sync with the above, because I think we need to add this as a deferred decl if and only if we did not emit a definition above.

Maybe what you should do is have a bool ShouldDeferDefinition that's initially false, but which you set true when you make the decision to emit it without an initializer. Or, actually, do you even need to delay the call to addDeferredDeclToEmit? Maybe you can just do that above as an alternative to building the null constant.

addDeferredDeclToEmit(GlobalDecl(&D));

// Ensure that the static local gets initialized by making sure the parent
// function gets emitted eventually.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I think you should actually be able to remove the rest of this function. What the existing code is doing is forcing the emission of the entire containing function that declares the static local, just so that it'll emit the constant initialization as a side-effect. But the deferred-decls approach is strictly superior: we've taught CodeGen to lazily emit the definition of the static local without any need to emit the entire containing function, so obviously we should just do that.

// If this value has an initializer, emit it.
if (D.getInit() && !isCudaSharedVar) {
if (D.getInit() && !isCudaSharedVar &&
!(D.isUsableInConstantExpressions(getContext()) && D.isStaticLocal())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is specific to static local variables, right? So I think D.isStaticLocal() is always true.

Can we skip emitting the global variable completely? What I'd like to see is that we just don't emit any IR associated with the static local at all if isUsableInConstantExpressions is true and the variable is never ODR-used. Like, it might be a little more complicated than that in corner cases — we'll have to emit the type in any case if it's variably-modified, and maybe these CUDA shared variables shouldn't be delayed (although maybe they're not usable in constant expressions anyway?) — but there's really no reason to emit a global declaration here that I can see.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think D.isStaticLocal() is always true.

I would expect that from a function named EmitStaticVarDecl, however It is not true at least for some OpenCL variables and if -fmerge-all-constants flag is passed. Same goes for getOrCreateStaticVarDecl since it is called above. I need to see what happens a bit more to avoid that check...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, sometimes we emit constant local variables as globals. Um... well, actually, there's no reason we couldn't be lazy about those as well if they're not ODR-used. There's no harm in that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to see what happens a bit more to avoid that check...

So, the problem is that EmitDeferred is very global variable/function-oriented. This is the reason I added the checks. Does it make sense to defer non-static variables that end up somehow being treated as static ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's no reason we couldn't be lazy about those as well if they're not ODR-used. There's no harm in that.

Ok, thanks


if (const auto *VD = dyn_cast<VarDecl>(D))
if (const auto *VD = dyn_cast<VarDecl>(D)) {
if (VD->isStaticLocal() && !getContext().shouldExternalize(D)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the deal with shouldExternalize?

Copy link
Contributor Author

@Fznamznon Fznamznon Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After my changes, I noticed one of CUDA tests failing. The only difference between old and new IR was that one of the variables was getting internal linkage with my changes. I found that CUDA device static variables used by host (stored in CUDADeviceVarODRUsedByHost field of ASTContext) are also put into DeferredDeclsToEmit at the beginning of EmitDeferred and they were emitted as globals. I'm not really an expert in CUDA support, so I tried to preserve the old behavior. shouldExternalize seems like an appropriate way to detect the case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I'm happy with that. Just leave a comment explaining what's going on to the best of your ability, please.

@cor3ntin
Copy link
Contributor

cor3ntin commented Dec 3, 2025

Sorry I missed this PR before - I think John's comment make sense and I don't have much to add - ping me when you need further reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:codegen IR generation bugs: mangling, exceptions, etc. clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Permitting static constexpr variables in consteval functions

4 participants