[Serialization] Stop demote var definition as declaration #172430
Conversation
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) ChangesClose #172241 This was meant to be a fix. Just during my debugging, I want to see the effect/reason/motivation of the action, then I simply remove the loop to demote var definition as declaration, and all tests suddenly passed. I've also tested our internal workloads and it looks well too. So I am wondering if this can be a real fix. The comment says “We should keep at most one definition on the chain.” But I am not sure if this is a strong restriction. To my mind model, I think it is fine as long as the definitions along the chain are equal. @zygoloid would you like to take a look? Why do we have this and do we have a counter example? @mpark @alexfh maybe you're interested to test this against your internal workloads to avoid break your internal code later. Full diff: https://github.com/llvm/llvm-project/pull/172430.diff 2 Files Affected:
diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index d245a111088d5..f1b7a3c937551 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -3645,19 +3645,6 @@ void ASTDeclReader::attachPreviousDeclImpl(ASTReader &Reader,
auto *PrevVD = cast<VarDecl>(Previous);
D->RedeclLink.setPrevious(PrevVD);
D->First = PrevVD->First;
-
- // We should keep at most one definition on the chain.
- // FIXME: Cache the definition once we've found it. Building a chain with
- // N definitions currently takes O(N^2) time here.
- if (VD->isThisDeclarationADefinition() == VarDecl::Definition) {
- for (VarDecl *CurD = PrevVD; CurD; CurD = CurD->getPreviousDecl()) {
- if (CurD->isThisDeclarationADefinition() == VarDecl::Definition) {
- Reader.mergeDefinitionVisibility(CurD, VD);
- VD->demoteThisDefinitionToDeclaration();
- break;
- }
- }
- }
}
static bool isUndeducedReturnType(QualType T) {
diff --git a/clang/test/Modules/pr172241.cppm b/clang/test/Modules/pr172241.cppm
new file mode 100644
index 0000000000000..3eb885e8b2d9f
--- /dev/null
+++ b/clang/test/Modules/pr172241.cppm
@@ -0,0 +1,47 @@
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// RUN: split-file %s %t
+//
+// RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/m.cppm -emit-module-interface -o %t/m.pcm
+// RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/use.cpp -fmodule-file=m=%t/m.pcm -emit-llvm -o - | FileCheck %t/use.cpp
+//
+// RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/m.cppm -emit-reduced-module-interface -o %t/m.pcm
+// RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/use.cpp -fmodule-file=m=%t/m.pcm -emit-llvm -o - | FileCheck %t/use.cpp
+
+//--- header.h
+#pragma once
+
+template <unsigned T>
+class Templ {
+public:
+ void lock() { __set_locked_bit(); }
+
+private:
+ static constexpr auto __set_locked_bit = [](){};
+};
+
+class JT {
+public:
+ ~JT() {
+ Templ<4> state;
+ state.lock();
+ }
+};
+
+//--- m.cppm
+module;
+#include "header.h"
+export module m;
+export struct M {
+ JT jt;
+};
+//--- use.cpp
+#include "header.h"
+import m;
+
+int main() {
+ M m;
+ return 0;
+}
+
+// CHECK: @_ZN5TemplILj4EE16__set_locked_bitE = {{.*}}linkonce_odr
|
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cppm,cpp -- clang/test/Modules/pr172241.cppm clang/lib/Serialization/ASTReaderDecl.cpp --diff_from_common_commit
View the diff from clang-format here.diff --git a/clang/lib/Serialization/ASTReaderDecl.cpp b/clang/lib/Serialization/ASTReaderDecl.cpp
index 71b86c90d..d119dc459 100644
--- a/clang/lib/Serialization/ASTReaderDecl.cpp
+++ b/clang/lib/Serialization/ASTReaderDecl.cpp
@@ -3637,7 +3637,7 @@ void ASTDeclReader::attachPreviousDeclImpl(ASTReader &Reader,
namespace clang {
-template<>
+template <>
void ASTDeclReader::attachPreviousDeclImpl(ASTReader &Reader,
Redeclarable<VarDecl> *D,
Decl *Previous, Decl *Canon) {
|
|
The purpose of this mechanism was to maintain an AST invariant that other parts of Clang may reasonably be relying on -- specifically that there is at most one definition in a redeclaration chain for a variable. Ultimately we have a choice: either we make the modules code attempt to build an AST that "looks like" a non-modules AST, for example with at most one definition per variable, function, or class, and the rest of Clang gets to assume that normal traditional C++ rules are in effect, or we allow the AST to directly represent things like a variable that has multiple definitions (or a class with multiple definitions or an inline function with multiple definitions) and the complexity associated with dealing with those things gets distributed across all parts of Clang and tools that consume its ASTs. So far we'e largely picked the first option. In part that's because Clang's modules system was originally a language extension, so keeping its impact contained made sense, and in part that's because the intent of the modules system has historically mostly been around getting to the same state that a non-modules compilation would reach, but faster. Maybe it's time to change that, given that modules is now a standard language feature. But I think this is probably something the Clang Area Team should consider and make a conscious decision on. (If we choose to directly represent the post-modules state in the AST -- for example, including the possibility of there being multiple definitions of entities in different TUs in the same AST -- I'd encourage that you also model the multiple TUs themselves explicitly, by creating multiple distinct |
3 participants
Close #172241
This wasn't meant to be a fix. Just during my debugging, I want to see the effect/reason/motivation of the action, then I simply remove the loop to demote var definition as declaration, and all tests suddenly passed. I've also tested our internal workloads and it looks well too.
So I am wondering if this can be a real fix. The comment says “We should keep at most one definition on the chain.” But I am not sure if this is a strong restriction. To my mind model, I think it is fine as long as the definitions along the chain are equal.
@zygoloid would you like to take a look? Why do we have this and do we have a counter example?
@mpark @alexfh maybe you're interested to test this against your internal workloads to avoid break your internal code later.