Skip to content
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

[Clang] call HandleImmediateInvocation before checking for immediate escacalating expressions #124414

Merged
merged 4 commits into from
Jan 27, 2025

Conversation

cor3ntin
Copy link
Contributor

HandleImmediateInvocation can call MarkExpressionAsImmediateEscalating and should always be called before CheckImmediateEscalatingFunctionDefinition.

However, we were not doing that in ActFunctionBody.

We simply move CheckImmediateEscalatingFunctionDefinition to PopExpressionEvaluationContext.

Fixes #119046

…escacalating expressions

HandleImmediateInvocation can call MarkExpressionAsImmediateEscalating
and should always be called before CheckImmediateEscalatingFunctionDefinition.

However, we were not doing that in `ActFunctionBody`.

We simply move CheckImmediateEscalatingFunctionDefinition to
PopExpressionEvaluationContext.

Fixes llvm#119046
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 25, 2025
@llvmbot
Copy link
Member

llvmbot commented Jan 25, 2025

@llvm/pr-subscribers-clang

Author: cor3ntin (cor3ntin)

Changes

HandleImmediateInvocation can call MarkExpressionAsImmediateEscalating and should always be called before CheckImmediateEscalatingFunctionDefinition.

However, we were not doing that in ActFunctionBody.

We simply move CheckImmediateEscalatingFunctionDefinition to PopExpressionEvaluationContext.

Fixes #119046


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

5 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+1)
  • (modified) clang/include/clang/Sema/Sema.h (+3-3)
  • (modified) clang/lib/Sema/SemaDecl.cpp (-1)
  • (modified) clang/lib/Sema/SemaExpr.cpp (+3)
  • (added) clang/test/CodeGenCXX/gh119046.cpp (+32)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e9fffddd507c66..f52c304f316692 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -991,6 +991,7 @@ Bug Fixes to C++ Support
 - Fixed assertions or false compiler diagnostics in the case of C++ modules for
   lambda functions or inline friend functions defined inside templates (#GH122493).
 - Clang now rejects declaring an alias template with the same name as its template parameter. (#GH123423)
+- Fix that some dependent immediate expressions did not cause immediate escalation (#GH119046)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 4d6e02fe2956e0..b3b24f04762717 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -13136,10 +13136,10 @@ class Sema final : public SemaBase {
     ~SynthesizedFunctionScope() {
       if (PushedCodeSynthesisContext)
         S.popCodeSynthesisContext();
-      if (auto *FD = dyn_cast<FunctionDecl>(S.CurContext)) {
+
+      if (auto *FD = dyn_cast<FunctionDecl>(S.CurContext))
         FD->setWillHaveBody(false);
-        S.CheckImmediateEscalatingFunctionDefinition(FD, S.getCurFunction());
-      }
+
       S.PopExpressionEvaluationContext();
       S.PopFunctionScopeInfo();
     }
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index ad49eac66e98e5..49f5d36383a881 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -16016,7 +16016,6 @@ Decl *Sema::ActOnFinishFunctionBody(Decl *dcl, Stmt *Body,
       if (!FD->isDeletedAsWritten())
         FD->setBody(Body);
       FD->setWillHaveBody(false);
-      CheckImmediateEscalatingFunctionDefinition(FD, FSI);
 
       if (getLangOpts().CPlusPlus14) {
         if (!FD->isInvalidDecl() && Body && !FD->isDependentContext() &&
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index d5273d463d7c01..6c0710c12b78a2 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -17876,6 +17876,9 @@ void Sema::PopExpressionEvaluationContext() {
   WarnOnPendingNoDerefs(Rec);
   HandleImmediateInvocations(*this, Rec);
 
+  if (auto *FD = dyn_cast<FunctionDecl>(CurContext); FD && getCurFunction())
+    CheckImmediateEscalatingFunctionDefinition(FD, getCurFunction());
+
   // Warn on any volatile-qualified simple-assignments that are not discarded-
   // value expressions nor unevaluated operands (those cases get removed from
   // this list by CheckUnusedVolatileAssignment).
diff --git a/clang/test/CodeGenCXX/gh119046.cpp b/clang/test/CodeGenCXX/gh119046.cpp
new file mode 100644
index 00000000000000..cad76879f08624
--- /dev/null
+++ b/clang/test/CodeGenCXX/gh119046.cpp
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -std=c++2a -triple x86_64-elf-gnu %s -emit-llvm -o - | FileCheck %s
+
+struct S {
+    consteval void operator()() {}
+};
+
+template <class Fn>
+constexpr void dispatch(Fn fn) {
+    fn();
+}
+
+template <class Visitor>
+struct value_visitor {
+    constexpr void operator()() { visitor(); }
+    Visitor&& visitor;
+};
+
+template <class Visitor>
+constexpr auto make_dispatch() {
+    return dispatch<value_visitor<S>>;
+}
+
+template <class Visitor>
+constexpr void visit(Visitor&&) {
+    make_dispatch<Visitor>();
+}
+
+void f() { visit(S{}); }
+
+// CHECK: define {{.*}} @_Z1fv
+// CHECK-NOT: define {{.*}} @_Z5visitI1SEvOT_
+// CHECK-NOT: define {{.*}} @_Z13make_dispatchI1SEDav

@cor3ntin cor3ntin merged commit 5815a31 into llvm:main Jan 27, 2025
5 of 7 checks passed
@cor3ntin cor3ntin deleted the gh119046 branch January 27, 2025 21:21
@nikic
Copy link
Contributor

nikic commented Jan 27, 2025

cor3ntin added a commit that referenced this pull request Jan 27, 2025
…mediate escacalating expressions" (#124646)

Reverts #124414

Turns out to be an important compile time regression, I'll come up with
a less disruptive approach
@cor3ntin
Copy link
Contributor Author

FYI this one seems like a fairly substantial compile-time regression for C++ code: https://llvm-compile-time-tracker.com/compare_clang.php?from=ad9da92cf6f735747ef04fd56937e1d76819e503&to=5815a311050ae218ebcda53adeee24ed96851943&stat=instructions%3Au&sortBy=interestingness

Thanks for noticing. I've reverted #124646
I think I know what the issue might be, I'll look into it soonish

github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 27, 2025
…king for immediate escacalating expressions" (#124646)

Reverts llvm/llvm-project#124414

Turns out to be an important compile time regression, I'll come up with
a less disruptive approach
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Clang] Linker error after immediate escalation
4 participants