-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[clang] Fix static local variables in consteval functions #156933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
3885133
995b574
e9f9de7
fedad4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -261,6 +261,7 @@ llvm::Constant *CodeGenModule::getOrCreateStaticVarDecl( | |
| if (llvm::Constant *ExistingGV = StaticLocalDeclMap[&D]) | ||
| return ExistingGV; | ||
|
|
||
| bool HasConstantInit = D.isUsableInConstantExpressions(getContext()); | ||
| QualType Ty = D.getType(); | ||
| assert(Ty->isConstantSizeType() && "VLAs can't be static"); | ||
|
|
||
|
|
@@ -275,18 +276,21 @@ llvm::Constant *CodeGenModule::getOrCreateStaticVarDecl( | |
| LangAS AS = GetGlobalVarAddressSpace(&D); | ||
| unsigned TargetAS = getContext().getTargetAddressSpace(AS); | ||
|
|
||
| llvm::Constant *Init = nullptr; | ||
| // OpenCL variables in local address space and CUDA shared | ||
| // variables cannot have an initializer. | ||
| llvm::Constant *Init = nullptr; | ||
| // Initializers for constant-initialized static locals are deferred. | ||
| 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 (!HasConstantInit) { | ||
| Init = EmitNullConstant(Ty); | ||
| } | ||
|
|
||
| llvm::GlobalVariable *GV = new llvm::GlobalVariable( | ||
| getModule(), LTy, Ty.isConstant(getContext()), Linkage, Init, Name, | ||
| nullptr, llvm::GlobalVariable::NotThreadLocal, TargetAS); | ||
|
|
||
| GV->setAlignment(getContext().getDeclAlign(&D).getAsAlign()); | ||
|
|
||
| if (supportsCOMDAT() && GV->isWeakForLinker()) | ||
|
|
@@ -309,6 +313,8 @@ llvm::Constant *CodeGenModule::getOrCreateStaticVarDecl( | |
| } | ||
|
|
||
| setStaticLocalDeclAddress(&D, Addr); | ||
| if (HasConstantInit && D.isStaticLocal()) | ||
| addDeferredDeclToEmit(GlobalDecl(&D)); | ||
|
|
||
| // Ensure that the static local gets initialized by making sure the parent | ||
| // function gets emitted eventually. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
|
@@ -336,6 +342,10 @@ llvm::Constant *CodeGenModule::getOrCreateStaticVarDecl( | |
| assert(isa<ObjCMethodDecl>(DC) && "unexpected parent code decl"); | ||
| } | ||
| if (GD.getDecl()) { | ||
| // Avoid producing declarations of consteval functions. | ||
| if (auto FD = dyn_cast<FunctionDecl>(GD.getDecl()); | ||
| FD && FD->isImmediateFunction()) | ||
| return Addr; | ||
| // Disable emission of the parent function for the OpenMP device codegen. | ||
| CGOpenMPRuntime::DisableAutoDeclareTargetRAII NoDeclTarget(*this); | ||
| (void)GetAddrOfGlobal(GD); | ||
|
|
@@ -406,6 +416,7 @@ void CodeGenFunction::EmitStaticVarDecl(const VarDecl &D, | |
| // declaration. This can happen when double-emitting function | ||
| // bodies, e.g. with complete and base constructors. | ||
| llvm::Constant *addr = CGM.getOrCreateStaticVarDecl(D, Linkage); | ||
|
|
||
| CharUnits alignment = getContext().getDeclAlign(&D); | ||
|
|
||
| // Store into LocalDeclMap before generating initializer to handle | ||
|
|
@@ -432,7 +443,8 @@ void CodeGenFunction::EmitStaticVarDecl(const VarDecl &D, | |
| bool isCudaSharedVar = getLangOpts().CUDA && getLangOpts().CUDAIsDevice && | ||
| D.hasAttr<CUDASharedAttr>(); | ||
| // If this value has an initializer, emit it. | ||
| if (D.getInit() && !isCudaSharedVar) { | ||
| if (D.getInit() && !isCudaSharedVar && | ||
| !(D.isUsableInConstantExpressions(getContext()) && D.isStaticLocal())) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function is specific to static local variables, right? So I think 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I would expect that from a function named
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So, the problem is that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok, thanks |
||
| ApplyAtomGroup Grp(getDebugInfo()); | ||
| var = AddInitializerToStaticVarDecl(D, var); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4461,8 +4461,14 @@ void CodeGenModule::EmitGlobalDefinition(GlobalDecl GD, llvm::GlobalValue *GV) { | |
| return EmitGlobalFunctionDefinition(GD, GV); | ||
| } | ||
|
|
||
| if (const auto *VD = dyn_cast<VarDecl>(D)) | ||
| if (const auto *VD = dyn_cast<VarDecl>(D)) { | ||
| if (VD->isStaticLocal() && !getContext().shouldExternalize(D)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the deal with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| CodeGenFunction(*this).AddInitializerToStaticVarDecl( | ||
| *VD, cast<llvm::GlobalVariable>(GV)); | ||
| return; | ||
| } | ||
| return EmitGlobalVarDefinition(VD, !VD->hasDefinition()); | ||
| } | ||
|
|
||
| llvm_unreachable("Invalid argument to EmitGlobalDefinition()"); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| // 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 const 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; | ||
| } | ||
|
|
||
| consteval const void * self_ref() { | ||
| static constexpr const void* b = &b; | ||
| return b; | ||
| } | ||
|
|
||
| // CHECK: @_ZZ1avE1b = linkonce_odr constant i32 3, comdat, align 4 | ||
| // CHECK: @_ZZ18returns48viastructvE1s = linkonce_odr constant %struct.S { i32 48 }, comdat, align 4 | ||
| // CHECK: @_ZZ3whyvE1a_0 = linkonce_odr constant i32 20, comdat, align 4 | ||
| // CHECK: @_ZZ12returnsarrayvE1a = linkonce_odr constant [3 x i32] [i32 10, i32 20, i32 30], comdat, align 4 | ||
| // CHECK: @_ZZ8self_refvE1b = linkonce_odr constant ptr @_ZZ8self_refvE1b, comdat, align 8 | ||
|
|
||
| // 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: ret i32 48 | ||
| // CHECK-NEXT: } | ||
|
|
||
| int f() { return *why(); } | ||
| // CHECK: define dso_local noundef i32 @_Z1fv() | ||
| // CHECK-NEXT: entry: | ||
| // CHECK-NEXT: %0 = load i32, ptr @_ZZ3whyvE1a_0, align 4 | ||
| // CHECK-NEXT: ret i32 %0 | ||
| // 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: } | ||
|
|
||
| using size_t = decltype(sizeof(void*)); | ||
| int usesself_ref() { | ||
| size_t b = (size_t)self_ref(); | ||
| return (int)b; | ||
| } | ||
| // CHECK: define dso_local noundef i32 @_Z12usesself_refv() | ||
| // CHECK-NEXT: entry: | ||
| // CHECK-NEXT: %b = alloca i64, align 8 | ||
| // CHECK-NEXT: store i64 ptrtoint (ptr @_ZZ8self_refvE1b to i64), ptr %b, align 8 | ||
| // CHECK-NEXT: %0 = load i64, ptr %b, align 8 | ||
| // CHECK-NEXT: %conv = trunc i64 %0 to i32 | ||
| // CHECK-NEXT: ret i32 %conv | ||
| // CHECK-NEXT: } | ||
|
|
||
| // CHECK-NOT: declare noundef ptr @_Z8self_refv() |
There was a problem hiding this comment.
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 ShouldDeferDefinitionthat'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 toaddDeferredDeclToEmit? Maybe you can just do that above as an alternative to building the null constant.