[CIR][CodeGen] Introduce CIR CXXSpecialMember attribute #1711

bruteforceboy · 2025-06-23T13:58:37Z

I think this one is self-explanatory, so I will not write much 🙂‍

Adding this attribute helps in optimizations like #1653, and using the attribute it's easy to create operations like cir.std.vector.ctor/cir.std.vector.dtor by just modifying IdiomRecognizer a bit. I believe it will also be useful for future optimizations. Finally, I updated quite a number of tests so they now reflect this attribute.

Please, let me know if you see any issues.

bcardosolopes

Thanks for adding more of the building blocks here. In the future it'd be nice to have a representation for struct/class more closer to C++ and it will probably be easier to extract this type of information, but to prevent over engineering early, it feels right to incrementally add pieces that can enable us to better analyze C++ code and make transformations easier to write.

Can you look into changing LifetimeChecker.cpp to use this attribute instead of the current AST approach?

bcardosolopes · 2025-06-23T21:41:27Z

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

@@ -1300,6 +1300,39 @@ def GlobalDtorAttr : CIR_GlobalCtorDtor<"Dtor", "dtor",
  "A function with this attribute excutes before module unloading"
 >;

+class CIR_CXXCtorDtor<string name, string attrMnemonic, string sum, string desc>


Maybe rename this to CIR_CXXSpecialMember? As we build on top of this I figure we'll want to look at operators and other things, so maybe relax on the name for now?

Agreed here, I see the value in recognizing things like push_back and emplace_back for other opts, and wonder if we should be doing that soon in a followup.

bcardosolopes · 2025-06-23T21:42:15Z

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

+  let summary = sum;
+  let description = desc;
+
+  let parameters = (ins "mlir::StringAttr":$name);


Do you plan to do anything with the string name? Seems like it would be more profitable long term to store a Type?

+1 Some kind of reference to the struct/class definition seems much more useful.

Yeah, storing the Type is much better, I have updated this.

bcardosolopes · 2025-06-23T21:44:14Z

clang/test/CIR/CodeGen/String.cpp

@@ -64,10 +64,10 @@ void test() {
 // CHECK-NEXT:   cir.store{{.*}} %arg1, %1 : !cir.ptr<!s8i>, !cir.ptr<!cir.ptr<!s8i>>
 // CHECK-NEXT:   %2 = cir.load{{.*}} %0 : !cir.ptr<!cir.ptr<!rec_String>>, !cir.ptr<!rec_String>
 // CHECK-NEXT:   %3 = cir.load{{.*}} %1 : !cir.ptr<!cir.ptr<!s8i>>, !cir.ptr<!s8i>
-// CHECK-NEXT:   cir.call @_ZN6StringC2EPKc(%2, %3) : (!cir.ptr<!rec_String>, !cir.ptr<!s8i>) -> ()
+// CHECK-NEXT:   cir.call @_ZN6StringC2EPKc(%2, %3) {cxx_ctor = #cir.cxx_ctor<"class String">} : (!cir.ptr<!rec_String>, !cir.ptr<!s8i>) -> ()


Instead of adding the attribute to the default attribute dict, perhaps have it proper in FuncOp and print something like ctor<type>? @xlauko since you have been cleaning up a lot of attribute things, any input here?

I wonder here too if this should be a property of the target function instead (that is, do this NOT on the call, but on the emission of _ZN6StringC2EPKc, then check it via indirection.

Yea, proper in FuncOp should be the way!

updated. The attributes are now attached directly to the FuncOp

bcardosolopes · 2025-06-23T21:45:48Z

clang/lib/CIR/CodeGen/CIRGenCall.cpp

@@ -424,7 +424,9 @@ RValue CIRGenFunction::emitCall(const CIRGenFunctionInfo &CallInfo,
                                const CallArgList &CallArgs,
                                cir::CIRCallOpInterface *callOrTryCall,
                                bool IsMustTail, mlir::Location loc,
-                                std::optional<const clang::CallExpr *> E) {
+                                std::optional<const clang::CallExpr *> E,
+                                std::optional<cir::CXXCtorAttr> cxxCtor,


Instead of adding two optionals, perhaps pass in a ArrayRef<Attribute> and loop over it while calling callLikeOp->setAttr(element)?

Yeah, I think this suggestion is a better idea. i was originally thinking we should do something where we capture the 'kind' better, but I think the suggestoin of just 'additionalAttrsToAdd' or something is better.

I have gotten rid of this now, since the attributes are now attached to the FuncOp

bcardosolopes · 2025-06-23T21:53:46Z

(@andykaylor @erichkeane @dkolsen-pgi in case you have any extra thoughts here)

andykaylor

Do we also want to set this attribute on cir.func definitions/declarations?

andykaylor · 2025-06-24T00:10:56Z

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

+  let summary = sum;
+  let description = desc;
+
+  let parameters = (ins "mlir::StringAttr":$name);


+1 Some kind of reference to the struct/class definition seems much more useful.

andykaylor · 2025-06-24T00:17:15Z

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

+  let summary = sum;
+  let description = desc;
+
+  let parameters = (ins "mlir::StringAttr":$name);


Would there be value in also storing the ctor/dtor kind (i.e. base vs. complete) or other traits (copy/move/default/etc.)?

I think maybe we can add traits incrementally, as required. For now, I have added copy & default because I use them in LifetimeCheck

erichkeane · 2025-06-24T18:34:15Z

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

@@ -1300,6 +1300,39 @@ def GlobalDtorAttr : CIR_GlobalCtorDtor<"Dtor", "dtor",
  "A function with this attribute excutes before module unloading"
 >;

+class CIR_CXXCtorDtor<string name, string attrMnemonic, string sum, string desc>


Agreed here, I see the value in recognizing things like push_back and emplace_back for other opts, and wonder if we should be doing that soon in a followup.

erichkeane · 2025-06-24T18:36:21Z

clang/lib/CIR/CodeGen/CIRGenCall.cpp

@@ -424,7 +424,9 @@ RValue CIRGenFunction::emitCall(const CIRGenFunctionInfo &CallInfo,
                                const CallArgList &CallArgs,
                                cir::CIRCallOpInterface *callOrTryCall,
                                bool IsMustTail, mlir::Location loc,
-                                std::optional<const clang::CallExpr *> E) {
+                                std::optional<const clang::CallExpr *> E,
+                                std::optional<cir::CXXCtorAttr> cxxCtor,


Yeah, I think this suggestion is a better idea. i was originally thinking we should do something where we capture the 'kind' better, but I think the suggestoin of just 'additionalAttrsToAdd' or something is better.

erichkeane · 2025-06-24T18:39:10Z

clang/lib/CIR/CodeGen/CIRGenExprCXX.cpp

+
+  auto cxxDtor = cir::CXXDtorAttr::get(
+      &getMLIRContext(),
+      getContext().getRecordType(DtorDecl->getParent()).getAsString());


IMO instead of counting on clang's to-string mechanism, I wonder if we could store a better way of identifying the kind. This is I believe going to include the template arguments along the way, which are going to be confusing.

IF what we are really looking for is something like "this is a std:: collection", I wonder if we should do the work to only do this in the case of DtorDecl->isInStdNamespace, and perhaps slightly more work to see if the base 'name' is correct (that is, only if DtorDecl->getParent()->getName() -is-in a certain list).

If we store the type instead, we can easily do this check if needed in an analysis (like this helper in

clangir/clang/lib/CIR/Dialect/Transforms/StdHelpers.cpp

Line 13 in 4820d60

bool isStdArrayType(mlir::Type t) {

), as used by the idiom recognizer (

clangir/clang/lib/CIR/Dialect/Transforms/IdiomRecognizer.cpp

Line 142 in 4820d60

static bool isIteratorInStdContainter(mlir::Type t) {

).

erichkeane · 2025-06-24T18:40:03Z

clang/test/CIR/CodeGen/String.cpp

@@ -64,10 +64,10 @@ void test() {
 // CHECK-NEXT:   cir.store{{.*}} %arg1, %1 : !cir.ptr<!s8i>, !cir.ptr<!cir.ptr<!s8i>>
 // CHECK-NEXT:   %2 = cir.load{{.*}} %0 : !cir.ptr<!cir.ptr<!rec_String>>, !cir.ptr<!rec_String>
 // CHECK-NEXT:   %3 = cir.load{{.*}} %1 : !cir.ptr<!cir.ptr<!s8i>>, !cir.ptr<!s8i>
-// CHECK-NEXT:   cir.call @_ZN6StringC2EPKc(%2, %3) : (!cir.ptr<!rec_String>, !cir.ptr<!s8i>) -> ()
+// CHECK-NEXT:   cir.call @_ZN6StringC2EPKc(%2, %3) {cxx_ctor = #cir.cxx_ctor<"class String">} : (!cir.ptr<!rec_String>, !cir.ptr<!s8i>) -> ()


I wonder here too if this should be a property of the target function instead (that is, do this NOT on the call, but on the emission of _ZN6StringC2EPKc, then check it via indirection.

github-actions · 2025-06-25T10:54:26Z

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

bruteforceboy · 2025-06-25T11:06:45Z

A few updates:

The attribute is now attached to the FuncOp (like ctor<type>) instead of the call
The attribute now stores Type instead of a string
I updated LifetimeChecker.cpp to make use of this attribute instead of the AST approach
I added a couple of traits for the ctor attribute, and I think we can extend as necessary in the future
CIR_CXXCtorDtor is now CIR_CXXSpecialMember
Updated a couple of tests to reflect the ctor/dtor attribute.

cc: @bcardosolopes, @erichkeane, @andykaylor

erichkeane

1 comment, else seems reasonable, I'm happy when the others are.

erichkeane · 2025-06-25T13:33:19Z

clang/lib/CIR/CodeGen/CIRGenFunction.cpp

-    else if (getLangOpts().CUDA && !getLangOpts().CUDAIsDevice &&
-             fd->hasAttr<CUDAGlobalAttr>())
+    } else if (getLangOpts().CUDA && !getLangOpts().CUDAIsDevice &&
+               fd->hasAttr<CUDAGlobalAttr>())


Suggested change

fd->hasAttr<CUDAGlobalAttr>())

fd->hasAttr<CUDAGlobalAttr>()) {

Our coding standard is that if anything in the 'if/else-if/else' list requires curleys, they all do, so just add them to this branch (and perhaps the 'else' at the end as a driveby).

bcardosolopes · 2025-06-26T18:38:54Z

clang/test/CIR/CodeGen/ctor-alias.cpp

@@ -37,4 +37,4 @@ B::B() {
 // CHECK:   %1 = cir.load %0 : !cir.ptr<!cir.ptr<!rec_B>>, !cir.ptr<!rec_B>
 // CHECK:   cir.return
 // CHECK: }
-// CHECK: cir.func private dso_local @_ZN1BC1Ev(!cir.ptr<!rec_B>) alias(@_ZN1BC2Ev)
+// CHECK: cir.func private dso_local @_ZN1BC1Ev(!cir.ptr<!rec_B>) ctor<!rec_B, default_ctor> alias(@_ZN1BC2Ev)


Perhaps use default instead of default_ctor.

Likewise for copy

bcardosolopes · 2025-06-26T18:44:01Z

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

+          "Functions with this attribute are CXX constructors"> {
+  let parameters = (ins "mlir::Type":$type,
+                        "bool":$is_default_constructor,
+                        "bool":$is_copy_constructor);


Seems like you want to use an enum kind here since those states cannot be true at the same time.

You're right, both cannot be true, but I think introducing an enum kind here makes it a bit complicated when introducing extra traits (in the future, if necessary).

bcardosolopes · 2025-06-26T18:44:30Z

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

+    `<`
+      $type `,` $is_default_constructor `,` $is_copy_constructor
+    `>`
+  }];


Please add a let description and explain the other params.

bcardosolopes · 2025-06-26T18:45:59Z

clang/include/clang/CIR/Dialect/IR/CIROps.td

-    OptionalAttr<AnyASTFunctionDeclAttr>:$ast
+    OptionalAttr<AnyASTFunctionDeclAttr>:$ast,
+    OptionalAttr<CXXCtorAttr>:$cxx_ctor,
+    OptionalAttr<CXXDtorAttr>:$cxx_dtor


A function cannot be ctor and dtor at the same time, perhaps both should inherit from a CIR_CXXSpecialMember instead such that we could have OptionalAttr<CXXSpecialMember>:$cxx_specialmember, which could be either?

Please, correct me if I am wrong, but I do not think we can use OptionalAttr<CXXSpecialMember>, since CIR_CXXSpecialMember is a class definition:

class CIR_CXXSpecialMember<string name, string attrMnemonic, string sum> : CIR_Attr<name, attrMnemonic> {...}

bcardosolopes · 2025-06-26T18:46:44Z

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

+    AttrBuilderWithInferredContext<(ins "mlir::Type":$type), [{
+      return $_get(type.getContext(), type);
+    }]>];
+}


We also need a CIR to CIR test for parsing/printing

bcardosolopes

Thanks for the update, one more round of suggestions

bruteforceboy requested review from lanza and bcardosolopes as code owners June 23, 2025 13:58

bcardosolopes reviewed Jun 23, 2025

View reviewed changes

andykaylor reviewed Jun 24, 2025

View reviewed changes

erichkeane reviewed Jun 24, 2025

View reviewed changes

add CXXCtorDtor attribute

8c7235c

bruteforceboy force-pushed the cxx-ctor-dtor branch from 68ff6ef to 8c7235c Compare June 25, 2025 10:51

Merge remote-tracking branch 'origin/main' into cxx-ctor-dtor

904732f

update

d4c51f3

bruteforceboy force-pushed the cxx-ctor-dtor branch from 77c8851 to d4c51f3 Compare June 25, 2025 11:47

erichkeane reviewed Jun 25, 2025

View reviewed changes

add curleys

c3480fd

bruteforceboy changed the title ~~[CIR][CodeGen] Introduce CIR CXXCtorDtor attribute~~ [CIR][CodeGen] Introduce CIR CXXSpecialMember attribute Jun 25, 2025

bcardosolopes reviewed Jun 26, 2025

View reviewed changes

bruteforceboy added 2 commits June 27, 2025 13:10

review comments & add cir-opt

f10b62f

Merge remote-tracking branch 'origin/main' into cxx-ctor-dtor

15a4a12

bruteforceboy force-pushed the cxx-ctor-dtor branch from ab65415 to c1f8a62 Compare June 27, 2025 11:48

update test & improve parsing

f6aaa4d

bruteforceboy force-pushed the cxx-ctor-dtor branch from c1f8a62 to f6aaa4d Compare June 27, 2025 12:01

	fd->hasAttr<CUDAGlobalAttr>())
	fd->hasAttr<CUDAGlobalAttr>()) {

[CIR][CodeGen] Introduce CIR CXXSpecialMember attribute #1711

Are you sure you want to change the base?

[CIR][CodeGen] Introduce CIR CXXSpecialMember attribute #1711

Uh oh!

Conversation

bruteforceboy commented Jun 23, 2025

Uh oh!

bcardosolopes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcardosolopes commented Jun 23, 2025

Uh oh!

andykaylor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bruteforceboy commented Jun 25, 2025

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

github-actions bot commented Jun 25, 2025 •

edited

Loading