Skip to content

Conversation

@andykaylor
Copy link
Contributor

@andykaylor andykaylor commented Sep 24, 2025

This change moves the getUsualDeleteParams function into the FunctionDecl class so that it can be shared between LLVM IR and CIR codegen.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen IR generation bugs: mangling, exceptions, etc. labels Sep 24, 2025
@llvmbot
Copy link
Member

llvmbot commented Sep 24, 2025

@llvm/pr-subscribers-clangir

@llvm/pr-subscribers-clang

Author: Andy Kaylor (andykaylor)

Changes

This change moves the getUsualDeleteParams function into the FunctionDecl class so that it can be shared between LLVM IR and CIR codegen. It also renames the function and associated structure to better reflect its use.


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

4 Files Affected:

  • (modified) clang/include/clang/AST/Decl.h (+3)
  • (modified) clang/include/clang/AST/ExprCXX.h (+8)
  • (modified) clang/lib/AST/Decl.cpp (+47)
  • (modified) clang/lib/CodeGen/CGExprCXX.cpp (+3-55)
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index d85d04d2a4d53..6894877ab5adf 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -80,6 +80,7 @@ class TypeAliasTemplateDecl;
 class UnresolvedSetImpl;
 class VarTemplateDecl;
 enum class ImplicitParamKind;
+struct DeleteParamInfo;
 
 // Holds a constraint expression along with a pack expansion index, if
 // expanded.
@@ -2646,6 +2647,8 @@ class FunctionDecl : public DeclaratorDecl,
   bool isTypeAwareOperatorNewOrDelete() const;
   void setIsTypeAwareOperatorNewOrDelete(bool IsTypeAwareOperator = true);
 
+  DeleteParamInfo getDeleteParamInfo() const;
+
   /// Compute the language linkage.
   LanguageLinkage getLanguageLinkage() const;
 
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index 9fedb230ce397..6b3706a7a84b9 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -2342,6 +2342,14 @@ struct ImplicitDeallocationParameters {
   SizedDeallocationMode PassSize;
 };
 
+/// The parameters to pass to a usual operator delete.
+struct DeleteParamInfo {
+  TypeAwareAllocationMode TypeAwareDelete = TypeAwareAllocationMode::No;
+  bool DestroyingDelete = false;
+  bool Size = false;
+  AlignedAllocationMode Alignment = AlignedAllocationMode::No;
+};
+
 /// Represents a new-expression for memory allocation and constructor
 /// calls, e.g: "new CXXNewExpr(foo)".
 class CXXNewExpr final
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index cd8e495e82c80..f7b80eb244191 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -3552,6 +3552,53 @@ void FunctionDecl::setIsTypeAwareOperatorNewOrDelete(bool IsTypeAware) {
   getASTContext().setIsTypeAwareOperatorNewOrDelete(this, IsTypeAware);
 }
 
+DeleteParamInfo FunctionDecl::getDeleteParamInfo() const {
+  DeleteParamInfo Params;
+
+  // This function should only be called for operator delete declarations.
+  assert(getDeclName().isAnyOperatorDelete());
+  if (!getDeclName().isAnyOperatorDelete())
+    return Params;
+
+  const FunctionProtoType *FPT = getType()->castAs<FunctionProtoType>();
+  auto AI = FPT->param_type_begin(), AE = FPT->param_type_end();
+
+  if (isTypeAwareOperatorNewOrDelete()) {
+    Params.TypeAwareDelete = TypeAwareAllocationMode::Yes;
+    assert(AI != AE);
+    ++AI;
+  }
+
+  // The first argument after the type-identity parameter (if any) is
+  // always a void* (or C* for a destroying operator delete for class
+  // type C).
+  ++AI;
+
+  // The next parameter may be a std::destroying_delete_t.
+  if (isDestroyingOperatorDelete()) {
+    assert(!isTypeAwareAllocation(Params.TypeAwareDelete));
+    Params.DestroyingDelete = true;
+    assert(AI != AE);
+    ++AI;
+  }
+
+  // Figure out what other parameters we should be implicitly passing.
+  if (AI != AE && (*AI)->isIntegerType()) {
+    Params.Size = true;
+    ++AI;
+  } else
+    assert(!isTypeAwareAllocation(Params.TypeAwareDelete));
+
+  if (AI != AE && (*AI)->isAlignValT()) {
+    Params.Alignment = AlignedAllocationMode::Yes;
+    ++AI;
+  } else
+    assert(!isTypeAwareAllocation(Params.TypeAwareDelete));
+
+  assert(AI == AE && "unexpected usual deallocation function parameter");
+  return Params;
+}
+
 LanguageLinkage FunctionDecl::getLanguageLinkage() const {
   return getDeclLanguageLinkage(*this);
 }
diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp
index a092b718412be..349319a571442 100644
--- a/clang/lib/CodeGen/CGExprCXX.cpp
+++ b/clang/lib/CodeGen/CGExprCXX.cpp
@@ -1376,58 +1376,6 @@ RValue CodeGenFunction::EmitBuiltinNewDeleteCall(const FunctionProtoType *Type,
   llvm_unreachable("predeclared global operator new/delete is missing");
 }
 
-namespace {
-/// The parameters to pass to a usual operator delete.
-struct UsualDeleteParams {
-  TypeAwareAllocationMode TypeAwareDelete = TypeAwareAllocationMode::No;
-  bool DestroyingDelete = false;
-  bool Size = false;
-  AlignedAllocationMode Alignment = AlignedAllocationMode::No;
-};
-}
-
-static UsualDeleteParams getUsualDeleteParams(const FunctionDecl *FD) {
-  UsualDeleteParams Params;
-
-  const FunctionProtoType *FPT = FD->getType()->castAs<FunctionProtoType>();
-  auto AI = FPT->param_type_begin(), AE = FPT->param_type_end();
-
-  if (FD->isTypeAwareOperatorNewOrDelete()) {
-    Params.TypeAwareDelete = TypeAwareAllocationMode::Yes;
-    assert(AI != AE);
-    ++AI;
-  }
-
-  // The first argument after the type-identity parameter (if any) is
-  // always a void* (or C* for a destroying operator delete for class
-  // type C).
-  ++AI;
-
-  // The next parameter may be a std::destroying_delete_t.
-  if (FD->isDestroyingOperatorDelete()) {
-    assert(!isTypeAwareAllocation(Params.TypeAwareDelete));
-    Params.DestroyingDelete = true;
-    assert(AI != AE);
-    ++AI;
-  }
-
-  // Figure out what other parameters we should be implicitly passing.
-  if (AI != AE && (*AI)->isIntegerType()) {
-    Params.Size = true;
-    ++AI;
-  } else
-    assert(!isTypeAwareAllocation(Params.TypeAwareDelete));
-
-  if (AI != AE && (*AI)->isAlignValT()) {
-    Params.Alignment = AlignedAllocationMode::Yes;
-    ++AI;
-  } else
-    assert(!isTypeAwareAllocation(Params.TypeAwareDelete));
-
-  assert(AI == AE && "unexpected usual deallocation function parameter");
-  return Params;
-}
-
 namespace {
   /// A cleanup to call the given 'operator delete' function upon abnormal
   /// exit from a new expression. Templated on a traits type that deals with
@@ -1494,7 +1442,7 @@ namespace {
       DeleteArgs.add(Traits::get(CGF, Ptr), FPT->getParamType(FirstNonTypeArg));
 
       // Figure out what other parameters we should be implicitly passing.
-      UsualDeleteParams Params;
+      DeleteParamInfo Params;
       if (NumPlacementArgs) {
         // A placement deallocation function is implicitly passed an alignment
         // if the placement allocation function was, but is never passed a size.
@@ -1505,7 +1453,7 @@ namespace {
       } else {
         // For a non-placement new-expression, 'operator delete' can take a
         // size and/or an alignment if it has the right parameters.
-        Params = getUsualDeleteParams(OperatorDelete);
+        Params = OperatorDelete->getDeleteParamInfo();
       }
 
       assert(!Params.DestroyingDelete &&
@@ -1838,7 +1786,7 @@ void CodeGenFunction::EmitDeleteCall(const FunctionDecl *DeleteFD,
   const auto *DeleteFTy = DeleteFD->getType()->castAs<FunctionProtoType>();
   CallArgList DeleteArgs;
 
-  auto Params = getUsualDeleteParams(DeleteFD);
+  auto Params = DeleteFD->getDeleteParamInfo();
   auto ParamTypeIt = DeleteFTy->param_type_begin();
 
   std::optional<llvm::AllocaInst *> TagAlloca;

@llvmbot
Copy link
Member

llvmbot commented Sep 24, 2025

@llvm/pr-subscribers-clang-codegen

Author: Andy Kaylor (andykaylor)

Changes

This change moves the getUsualDeleteParams function into the FunctionDecl class so that it can be shared between LLVM IR and CIR codegen. It also renames the function and associated structure to better reflect its use.


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

4 Files Affected:

  • (modified) clang/include/clang/AST/Decl.h (+3)
  • (modified) clang/include/clang/AST/ExprCXX.h (+8)
  • (modified) clang/lib/AST/Decl.cpp (+47)
  • (modified) clang/lib/CodeGen/CGExprCXX.cpp (+3-55)
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index d85d04d2a4d53..6894877ab5adf 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -80,6 +80,7 @@ class TypeAliasTemplateDecl;
 class UnresolvedSetImpl;
 class VarTemplateDecl;
 enum class ImplicitParamKind;
+struct DeleteParamInfo;
 
 // Holds a constraint expression along with a pack expansion index, if
 // expanded.
@@ -2646,6 +2647,8 @@ class FunctionDecl : public DeclaratorDecl,
   bool isTypeAwareOperatorNewOrDelete() const;
   void setIsTypeAwareOperatorNewOrDelete(bool IsTypeAwareOperator = true);
 
+  DeleteParamInfo getDeleteParamInfo() const;
+
   /// Compute the language linkage.
   LanguageLinkage getLanguageLinkage() const;
 
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
index 9fedb230ce397..6b3706a7a84b9 100644
--- a/clang/include/clang/AST/ExprCXX.h
+++ b/clang/include/clang/AST/ExprCXX.h
@@ -2342,6 +2342,14 @@ struct ImplicitDeallocationParameters {
   SizedDeallocationMode PassSize;
 };
 
+/// The parameters to pass to a usual operator delete.
+struct DeleteParamInfo {
+  TypeAwareAllocationMode TypeAwareDelete = TypeAwareAllocationMode::No;
+  bool DestroyingDelete = false;
+  bool Size = false;
+  AlignedAllocationMode Alignment = AlignedAllocationMode::No;
+};
+
 /// Represents a new-expression for memory allocation and constructor
 /// calls, e.g: "new CXXNewExpr(foo)".
 class CXXNewExpr final
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index cd8e495e82c80..f7b80eb244191 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -3552,6 +3552,53 @@ void FunctionDecl::setIsTypeAwareOperatorNewOrDelete(bool IsTypeAware) {
   getASTContext().setIsTypeAwareOperatorNewOrDelete(this, IsTypeAware);
 }
 
+DeleteParamInfo FunctionDecl::getDeleteParamInfo() const {
+  DeleteParamInfo Params;
+
+  // This function should only be called for operator delete declarations.
+  assert(getDeclName().isAnyOperatorDelete());
+  if (!getDeclName().isAnyOperatorDelete())
+    return Params;
+
+  const FunctionProtoType *FPT = getType()->castAs<FunctionProtoType>();
+  auto AI = FPT->param_type_begin(), AE = FPT->param_type_end();
+
+  if (isTypeAwareOperatorNewOrDelete()) {
+    Params.TypeAwareDelete = TypeAwareAllocationMode::Yes;
+    assert(AI != AE);
+    ++AI;
+  }
+
+  // The first argument after the type-identity parameter (if any) is
+  // always a void* (or C* for a destroying operator delete for class
+  // type C).
+  ++AI;
+
+  // The next parameter may be a std::destroying_delete_t.
+  if (isDestroyingOperatorDelete()) {
+    assert(!isTypeAwareAllocation(Params.TypeAwareDelete));
+    Params.DestroyingDelete = true;
+    assert(AI != AE);
+    ++AI;
+  }
+
+  // Figure out what other parameters we should be implicitly passing.
+  if (AI != AE && (*AI)->isIntegerType()) {
+    Params.Size = true;
+    ++AI;
+  } else
+    assert(!isTypeAwareAllocation(Params.TypeAwareDelete));
+
+  if (AI != AE && (*AI)->isAlignValT()) {
+    Params.Alignment = AlignedAllocationMode::Yes;
+    ++AI;
+  } else
+    assert(!isTypeAwareAllocation(Params.TypeAwareDelete));
+
+  assert(AI == AE && "unexpected usual deallocation function parameter");
+  return Params;
+}
+
 LanguageLinkage FunctionDecl::getLanguageLinkage() const {
   return getDeclLanguageLinkage(*this);
 }
diff --git a/clang/lib/CodeGen/CGExprCXX.cpp b/clang/lib/CodeGen/CGExprCXX.cpp
index a092b718412be..349319a571442 100644
--- a/clang/lib/CodeGen/CGExprCXX.cpp
+++ b/clang/lib/CodeGen/CGExprCXX.cpp
@@ -1376,58 +1376,6 @@ RValue CodeGenFunction::EmitBuiltinNewDeleteCall(const FunctionProtoType *Type,
   llvm_unreachable("predeclared global operator new/delete is missing");
 }
 
-namespace {
-/// The parameters to pass to a usual operator delete.
-struct UsualDeleteParams {
-  TypeAwareAllocationMode TypeAwareDelete = TypeAwareAllocationMode::No;
-  bool DestroyingDelete = false;
-  bool Size = false;
-  AlignedAllocationMode Alignment = AlignedAllocationMode::No;
-};
-}
-
-static UsualDeleteParams getUsualDeleteParams(const FunctionDecl *FD) {
-  UsualDeleteParams Params;
-
-  const FunctionProtoType *FPT = FD->getType()->castAs<FunctionProtoType>();
-  auto AI = FPT->param_type_begin(), AE = FPT->param_type_end();
-
-  if (FD->isTypeAwareOperatorNewOrDelete()) {
-    Params.TypeAwareDelete = TypeAwareAllocationMode::Yes;
-    assert(AI != AE);
-    ++AI;
-  }
-
-  // The first argument after the type-identity parameter (if any) is
-  // always a void* (or C* for a destroying operator delete for class
-  // type C).
-  ++AI;
-
-  // The next parameter may be a std::destroying_delete_t.
-  if (FD->isDestroyingOperatorDelete()) {
-    assert(!isTypeAwareAllocation(Params.TypeAwareDelete));
-    Params.DestroyingDelete = true;
-    assert(AI != AE);
-    ++AI;
-  }
-
-  // Figure out what other parameters we should be implicitly passing.
-  if (AI != AE && (*AI)->isIntegerType()) {
-    Params.Size = true;
-    ++AI;
-  } else
-    assert(!isTypeAwareAllocation(Params.TypeAwareDelete));
-
-  if (AI != AE && (*AI)->isAlignValT()) {
-    Params.Alignment = AlignedAllocationMode::Yes;
-    ++AI;
-  } else
-    assert(!isTypeAwareAllocation(Params.TypeAwareDelete));
-
-  assert(AI == AE && "unexpected usual deallocation function parameter");
-  return Params;
-}
-
 namespace {
   /// A cleanup to call the given 'operator delete' function upon abnormal
   /// exit from a new expression. Templated on a traits type that deals with
@@ -1494,7 +1442,7 @@ namespace {
       DeleteArgs.add(Traits::get(CGF, Ptr), FPT->getParamType(FirstNonTypeArg));
 
       // Figure out what other parameters we should be implicitly passing.
-      UsualDeleteParams Params;
+      DeleteParamInfo Params;
       if (NumPlacementArgs) {
         // A placement deallocation function is implicitly passed an alignment
         // if the placement allocation function was, but is never passed a size.
@@ -1505,7 +1453,7 @@ namespace {
       } else {
         // For a non-placement new-expression, 'operator delete' can take a
         // size and/or an alignment if it has the right parameters.
-        Params = getUsualDeleteParams(OperatorDelete);
+        Params = OperatorDelete->getDeleteParamInfo();
       }
 
       assert(!Params.DestroyingDelete &&
@@ -1838,7 +1786,7 @@ void CodeGenFunction::EmitDeleteCall(const FunctionDecl *DeleteFD,
   const auto *DeleteFTy = DeleteFD->getType()->castAs<FunctionProtoType>();
   CallArgList DeleteArgs;
 
-  auto Params = getUsualDeleteParams(DeleteFD);
+  auto Params = DeleteFD->getDeleteParamInfo();
   auto ParamTypeIt = DeleteFTy->param_type_begin();
 
   std::optional<llvm::AllocaInst *> TagAlloca;

@cor3ntin cor3ntin requested a review from ojhunt September 24, 2025 17:23
@ojhunt
Copy link
Contributor

ojhunt commented Sep 24, 2025

This looks fine, but what is the reason for the rename?

@andykaylor
Copy link
Contributor Author

This looks fine, but what is the reason for the rename?

It just seemed more readable to me, since the structure returned is providing information about the parameters rather than the parameters. I'm not particularly attached to the renaming if you want to keep the old name for some reason.

@ojhunt
Copy link
Contributor

ojhunt commented Sep 24, 2025

This looks fine, but what is the reason for the rename?

It just seemed more readable to me, since the structure returned is providing information about the parameters rather than the parameters. I'm not particularly attached to the renaming if you want to keep the old name for some reason.

I agree that the existing naming is weird, but it is that because "usual deallocation function" is the specific language from the C++ specification.

@andykaylor
Copy link
Contributor Author

I agree that the existing naming is weird, but it is that because "usual deallocation function" is the specific language from the C++ specification.

OK, thanks. I missed that. I'll change the names back.

@andykaylor
Copy link
Contributor Author

ping

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems reasonable to me.

Copy link
Contributor

@ojhunt ojhunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!!

Sorry! I had vacation + week of plague - and then completely dropped this. Super sorry!

@@ -2646,6 +2647,8 @@ class FunctionDecl : public DeclaratorDecl,
bool isTypeAwareOperatorNewOrDelete() const;
void setIsTypeAwareOperatorNewOrDelete(bool IsTypeAwareOperator = true);

UsualDeleteParams getUsualDeleteParams() const;
Copy link
Contributor

@ojhunt ojhunt Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erichkeane is it reasonable to have this here but the struct definition be in a separate header? Moving the definition to Decl.h, requires a bunch of other related definitions to be moved - I think it would be reasonable to do so if we're going to make this present directly in Decl?

@andykaylor I think this should be an optional return - e.g. std::optional<UsualDeleteParams> or we could smooth out the declaration w/o the definition with bool getUsualDeleteParams(UsualDeleteParams *) const. I'd prefer optional<UsualDeleteParams> given the alternation of true and false meaning success or fail in different places.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a concern with it. Though, you can't really call the function at all without also including ExprCXX.h of course (since it returns a type requiring completeness).

We do this a few other places of course.

The std::optional might be nice, but what state is it supposed to represent? Are you just meaning "for a non-delete decl"? At that point, an assert is probably even better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andykaylor I think this should be an optional return - e.g. std::optional<UsualDeleteParams> or we could smooth out the declaration w/o the definition with bool getUsualDeleteParams(UsualDeleteParams *) const. I'd prefer optional<UsualDeleteParams> given the alternation of true and false meaning success or fail in different places.

So rather than assert and return the default structure if it's called for a non-delete function it would just return std::nullopt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's my preference? Though I realize I was thinking we could remove the individual queries but that would require the addition of an equivalent struct for new - but that might be nicer in general? There are existing structs that are similar and maybe we could make things generally nicer by reducing the number of variations of these structures?

Such a change would not be necessary as part of this PR of course - on the other hand maybe an assertion is perfectly reasonable? That is consistent with lots of other code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andykaylor actually yeah, lets stick with an assertion for now - changing the structure of new/delete queries is a different task from this one. r=me.

This change moves the getUsualDeleteParams function into the FunctionDecl
class so that it can be shared between LLVM IR and CIR codegen. It also
renames the function and associated structure to better reflect its use.
@andykaylor
Copy link
Contributor Author

The changes I made to CIR in #160574 conflict with this. I'm working on a small change to align the CIR code with the refactoring in this PR. I will update as soon as my local build and test finish.

@llvmbot llvmbot added the ClangIR Anything related to the ClangIR project label Oct 1, 2025
@andykaylor andykaylor merged commit 1e4d4bb into llvm:main Oct 1, 2025
10 checks passed
@andykaylor andykaylor deleted the usual-delete-args branch October 1, 2025 22:14
return Params;

const FunctionProtoType *FPT = getType()->castAs<FunctionProtoType>();
auto AI = FPT->param_type_begin(), AE = FPT->param_type_end();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does AI and AE stand for? Can we get more descriptive names?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats pre-existing (though different capitalization, but I'll guess it is supposed to be 'args-iterator' and 'args-end'. But thats a guess, and also kinda wrong, since these are parameters not args.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure you can squint and say it is existing code but if no one understand what the variable names mean, it probably makes sense to just rename them. Your idea seems plausible.

mahesh-attarde pushed a commit to mahesh-attarde/llvm-project that referenced this pull request Oct 3, 2025
This change moves the getUsualDeleteParams function into the
FunctionDecl class so that it can be shared between LLVM IR and CIR
codegen.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:codegen IR generation bugs: mangling, exceptions, etc. clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category ClangIR Anything related to the ClangIR project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants