[Clang] Eagerly instantiate used constexpr function upon definition. #73463
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang Author: cor3ntin (cor3ntin) ChangesDespite CWG2497 not being resolved, it is reasonable to expect the following code to compile (and which is supported by other compilers) template<typename T> constexpr T f();
constexpr int g() { return f<int>(); } // #<!-- -->1
template<typename T> constexpr T f() { return 123; }
int k[g()];
// #<!-- -->2To that end, we eagerly instantiate all referenced specializations of constexpr functions when they are defined. We maintain a map of (pattern, [instantiations]) independant of We should apply the same logic to constexpr variables, but I wanted to keep the PR small. Fixes #73232 Full diff: https://github.com/llvm/llvm-project/pull/73463.diff 7 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 29a06d0f713f588..b34b66ec7dcabc2 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -762,6 +762,11 @@ Bug Fixes to C++ Support
completes (except deduction guides). Fixes:
(`#59827 <https://github.com/llvm/llvm-project/issues/59827>`_)
+- Clang now immediately instantiates function template specializations
+ at the end of the definition of the corresponding function template
+ when the definition appears after the first point of instantiation.
+ (`#73232 <https://github.com/llvm/llvm-project/issues/73232>`_)
+
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
- Fixed an import failure of recursive friend class template.
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index f7c9d0e2e6412b7..091e1e3b4c1fd64 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -59,6 +59,7 @@
#include "clang/Sema/TypoCorrection.h"
#include "clang/Sema/Weak.h"
#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/SmallBitVector.h"
#include "llvm/ADT/SmallPtrSet.h"
@@ -10078,6 +10079,12 @@ class Sema final {
/// but have not yet been performed.
std::deque<PendingImplicitInstantiation> PendingInstantiations;
+ /// Track constexpr functions referenced before they are (lexically) defined.
+ /// The key is the pattern, associated with a list of specialisations that
+ /// need to be instantiated when the pattern is defined.
+ llvm::DenseMap<NamedDecl *, SmallVector<NamedDecl *>>
+ PendingInstantiationsOfConstexprEntities;
+
/// Queue of implicit template instantiations that cannot be performed
/// eagerly.
SmallVector<PendingImplicitInstantiation, 1> LateParsedInstantiations;
@@ -10396,6 +10403,10 @@ class Sema final {
bool Recursive = false,
bool DefinitionRequired = false,
bool AtEndOfTU = false);
+
+ void InstantiateFunctionTemplateSpecializations(
+ SourceLocation PointOfInstantiation, FunctionDecl *Template);
+
VarTemplateSpecializationDecl *BuildVarTemplateInstantiation(
VarTemplateDecl *VarTemplate, VarDecl *FromVar,
const TemplateArgumentList &TemplateArgList,
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 23dd8ae15c16583..1030ba0d21b1906 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -16255,6 +16255,9 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
if (FD && !FD->isDeleted())
checkTypeSupport(FD->getType(), FD->getLocation(), FD);
+ if (FD && FD->isConstexpr() && FD->isTemplated())
+ InstantiateFunctionTemplateSpecializations(FD->getEndLoc(), FD);
+
return dcl;
}
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index fc39d6149c1cc65..37b0d0eed35845c 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -19047,12 +19047,17 @@ void Sema::MarkFunctionReferenced(SourceLocation Loc, FunctionDecl *Func,
CodeSynthesisContexts.size())
PendingLocalImplicitInstantiations.push_back(
std::make_pair(Func, PointOfInstantiation));
- else if (Func->isConstexpr())
+ else if (Func->isConstexpr()) {
// Do not defer instantiations of constexpr functions, to avoid the
// expression evaluator needing to call back into Sema if it sees a
// call to such a function.
InstantiateFunctionDefinition(PointOfInstantiation, Func);
- else {
+ if (!Func->isDefined()) {
+ PendingInstantiationsOfConstexprEntities
+ [Func->getTemplateInstantiationPattern()->getCanonicalDecl()]
+ .push_back(Func);
+ }
+ } else {
Func->setInstantiationIsPending(true);
PendingInstantiations.push_back(
std::make_pair(Func, PointOfInstantiation));
diff --git a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
index 08f4ba00fc9f7de..9afbf139a1c2b88 100644
--- a/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -6481,6 +6481,23 @@ void Sema::PerformPendingInstantiations(bool LocalOnly) {
PendingInstantiations.swap(delayedPCHInstantiations);
}
+// Instantiate all referenced specializations of the given function template
+// definition. This make sure that function template that are defined after the
+// point of instantiation of their used can be evaluated after they are defined.
+// see CWG2497.
+void Sema::InstantiateFunctionTemplateSpecializations(
+ SourceLocation PointOfInstantiation, FunctionDecl *Tpl) {
+ auto It =
+ PendingInstantiationsOfConstexprEntities.find(Tpl->getCanonicalDecl());
+ if (It == PendingInstantiationsOfConstexprEntities.end())
+ return;
+ for (NamedDecl *Fun : It->second) {
+ InstantiateFunctionDefinition(PointOfInstantiation,
+ cast<FunctionDecl>(Fun));
+ }
+ PendingInstantiationsOfConstexprEntities.erase(It);
+}
+
void Sema::PerformDependentDiagnostics(const DeclContext *Pattern,
const MultiLevelTemplateArgumentList &TemplateArgs) {
for (auto *DD : Pattern->ddiags()) {
diff --git a/clang/test/SemaCXX/cxx2b-consteval-propagate.cpp b/clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
index 531a62622873357..a2b4039a31db778 100644
--- a/clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
+++ b/clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
@@ -105,13 +105,15 @@ template <typename T>
constexpr int f(T t);
auto a = &f<char>;
-auto b = &f<int>; // expected-error {{immediate function 'f<int>' used before it is defined}} \
- // expected-note {{in instantiation of function template specialization}}
+auto b = &f<int>; // expected-error {{immediate function 'f<int>' used before it is defined}}
template <typename T>
constexpr int f(T t) { // expected-note {{'f<int>' defined here}}
return id(t); // expected-note {{'f<int>' is an immediate function because its body contains a call to a consteval function 'id' and that call is not a constant expression}}
-}
+} // expected-note {{in instantiation of function template specialization}}
+
+
+
}
namespace constructors {
diff --git a/clang/test/SemaTemplate/instantiate-used-constexpr-function.cpp b/clang/test/SemaTemplate/instantiate-used-constexpr-function.cpp
new file mode 100644
index 000000000000000..57353d681ac7708
--- /dev/null
+++ b/clang/test/SemaTemplate/instantiate-used-constexpr-function.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+namespace GH73232 {
+
+template <typename _CharT>
+struct basic_string {
+ constexpr void _M_construct();
+ constexpr basic_string() {
+ _M_construct();
+ }
+};
+
+basic_string<char> a;
+
+template <typename _CharT>
+constexpr void basic_string<_CharT>::_M_construct(){}
+constexpr basic_string<char> z{};
+
+template <typename T>
+constexpr void g(T);
+
+constexpr int f() { g(0); return 0; }
+
+template <typename T>
+constexpr void g(T) {}
+
+constexpr int z = f();
+
+}
|
cor3ntin
force-pushed
the
corentin/gh73232
branch
from
ea67650
to
80fc233
Compare
|
Does this have any compile-time impact? |
Nope, should be ~free. It does add 1 DenseMap lookup per constexpr function definition, which ought to be negligible. |
|
I think the new map should be unnecessary: If you want/need to keep the map approach, you'll need to also deal with the case where the template is declared in a PCH and then defined somewhere that includes the PCH, which will mean serializing and deserializing the map contents. |
I don't think that's viable for non-template but templated entities like member function in a class template. Am I missing something?
I did not consider that :( |
Hmm, good point, we don't have a way to walk the specializations of a templated function that's not a function template. (You'd need to walk all the instantiations of the enclosing template and check to see if the corresponding member function has a pending instantiation, or something like that, which sounds painful. Friends defined in class templates would probably be especially problematic.) Let's use your map approach then :-) |
cor3ntin
force-pushed
the
corentin/gh73232
branch
from
80fc233
to
b072438
Compare
|
@zygoloid It took me a while, but I think I got it; what do you think of direction ? (description in the last commit message) |
cor3ntin
force-pushed
the
corentin/gh73232
branch
from
b072438
to
841af0b
Compare
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Revert these added blank lines.
Despite CWG2497 not being resolved, it is reasonable to expect the following
code to compile (and which is supported by other compilers)
```cpp
template<typename T> constexpr T f();
constexpr int g() { return f<int>(); } // #1
template<typename T> constexpr T f() { return 123; }
int k[g()];
// #2
```
To that end, we eagerly instantiate all referenced
specializations of constexpr functions when they are defined.
We maintain a map of (pattern, [instantiations]) independant of
`PendingInstantiations` to avoid having to iterate that list after
each function definition.
We should apply the same logic to constexpr variables,
but I wanted to keep the PR small.
Fixes llvm#73232
I wanted to avoid loading eagerly all function templates and their instantiations - even if ultimately `PendingInstantiations` - which contains the same information is going to be loaded at the end of the TU. So instead, when we find the definition of a constexpr function template we load the associated instantiation from the external modules at that point. I'm not sure there is a better way to get to a DeclID from a Decl* than to iterate over all redeclarations? I still need to cleanup but I'd like feedback on direction first!
cor3ntin
force-pushed
the
corentin/gh73232
branch
from
841af0b
to
75728d3
Compare
I'll try to get to it, yes! |
| // Build a record containing all pending instantiations of constexpr | ||
| // entities. | ||
| RecordData PendingInstantiationsOfConstexprEntities; | ||
| for (const auto &I : SemaRef.PendingInstantiationsOfConstexprEntities) { |
There was a problem hiding this comment.
In case this will be relanded in some form:
This loop uses arbitrary hashtable order, leading to nondeterministic PCM output.
Despite CWG2497 not being resolved, it is reasonable to expect the following code to compile (and which is supported by other compilers)
To that end, we eagerly instantiate all referenced specializations of constexpr functions when they are defined.
We maintain a map of (pattern, [instantiations]) independant of
PendingInstantiationsto avoid having to iterate that list after each function definition.We should apply the same logic to constexpr variables, but I wanted to keep the PR small.
Fixes #73232