[C++20] [Modules] Don't generate the defintition for non-const available external variables

[ChuanqiXu9]

Close #93497

The root cause of the problem is, we mark the variable from other
modules as constnant in LLVM incorrectly. This patch fixes this problem
by not emitting the defintition for non-const available external
variables. Since the non const available externally variable is not
helpful to the optimization.

[llvmbot]

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang-modules

Author: Chuanqi Xu (ChuanqiXu9)

Changes

Close #93497

The root cause of the problem is, we mark the variable from other modules as constnant in LLVM incorrectly. This patch fixes this problem and only mark the variable from other modules as constant if its initializer is const.

---

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

2 Files Affected:

• (modified) clang/lib/CodeGen/CodeGenModule.cpp (+2)
• (added) clang/test/Modules/pr93497.cppm (+81)

diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index e4774a587707a..5596a6708e4a1 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -5492,6 +5492,8 @@ void CodeGenModule::EmitGlobalVarDefinition(const VarDecl *D,
 
   // If it is safe to mark the global 'constant', do so now.
   GV->setConstant(!NeedsGlobalCtor && !NeedsGlobalDtor &&
+                  (!IsDefinitionAvailableExternally ||
+                   D->hasConstantInitialization()) &&
                   D->getType().isConstantStorage(getContext(), true, true));
 
   // If it is in a read-only section, mark it 'constant'.
diff --git a/clang/test/Modules/pr93497.cppm b/clang/test/Modules/pr93497.cppm
new file mode 100644
index 0000000000000..dc0d8eb462e27
--- /dev/null
+++ b/clang/test/Modules/pr93497.cppm
@@ -0,0 +1,81 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 %t/mod.cppm \
+// RUN:     -emit-module-interface -o %t/mod.pcm
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 %t/use.cpp \
+// RUN:     -fmodule-file=mod=%t/mod.pcm -emit-llvm \
+// RUN:     -o - | FileCheck %t/use.cpp
+
+//--- mod.cppm
+export module mod;
+
+export struct Thing {
+    static const Thing One;
+    explicit Thing(int raw) :raw(raw) { }
+    int raw;
+};
+
+const Thing Thing::One = Thing(1);
+
+export struct C {
+    int value;
+};
+export const C ConstantValue = {1};
+
+export const C *ConstantPtr = &ConstantValue;
+
+C NonConstantValue = {1};
+export const C &ConstantRef = NonConstantValue;
+
+//--- use.cpp
+import mod;
+
+int consume(int);
+int consumeC(C);
+
+extern "C" __attribute__((noinline)) inline int unneeded() {
+    return consume(43);
+}
+
+extern "C" __attribute__((noinline)) inline int needed() {
+    return consume(43);
+}
+
+int use() {
+    Thing t1 = Thing::One;
+    return consume(t1.raw);
+}
+
+int use2() {
+    if (ConstantValue.value)
+        return consumeC(ConstantValue);
+    return unneeded();
+}
+
+int use3() {
+    if (ConstantPtr->value)
+        return consumeC(*ConstantPtr);
+    return needed();
+}
+
+int use4() {
+    if (ConstantRef.value)
+        return consumeC(ConstantRef);
+    return needed();
+}
+
+// CHECK-NOT: @_ZNW3mod5Thing3OneE = {{.*}}constant
+// CHECK: @_ZW3mod13ConstantValue ={{.*}}available_externally{{.*}} constant 
+
+// Check that the middle end can optimize the program by the constant information.
+// CHECK-NOT: @unneeded(
+
+// Check that the use of ConstantPtr won't get optimized incorrectly.
+// CHECK-LABEL: @_Z4use3v(
+// CHECK: @needed(
+
+// Check that the use of ConstantRef won't get optimized incorrectly.
+// CHECK-LABEL: @_Z4use4v(
+// CHECK: @needed(

[github-actions]

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

[efriedma-quic]

I think if a variable is GVA_AvailableExternally, and we can't emit a constant, we should just completely skip emitting the definition: there isn't any point to emitting an available_externally definition that doesn't actually contain any information the optimizer can use.

Not sure off the top of my head where that check belongs; might be okay to just stick it into EmitGlobalVarDefinition itself.

[ChuanqiXu9]

I think if a variable is GVA_AvailableExternally, and we can't emit a constant, we should just completely skip emitting the definition: there isn't any point to emitting an available_externally definition that doesn't actually contain any information the optimizer can use.

Not sure off the top of my head where that check belongs; might be okay to just stick it into EmitGlobalVarDefinition itself.

Neat suggestion! I've applied it and the generated code looks better.

[efriedma-quic]

Should we check for needsDestruction()? (For example, consider a struct with a constexpr constructor, but a non-trivial destructor.)

[ChuanqiXu9]

(we think) the available externally variables don't need ctor or dtor in the current TU. I mentioned this in the comment.

[efriedma-quic]

No, I mean, please make sure we don't emit an available_externally definition in that case.

[ChuanqiXu9]

Got it. Makes sense. Done.

[efriedma-quic]

LGTM