From a4e344643f8b7db6d6357fda7fba7d0d3da6f130 Mon Sep 17 00:00:00 2001 From: Alexandru Lorinti Date: Mon, 7 Jul 2025 15:22:20 +0300 Subject: [PATCH 1/3] avoiding unnecessary copies --- mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp index 8cc831441810..37968d863954 100644 --- a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp +++ b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp @@ -393,7 +393,7 @@ void DefGen::emitCheckedBuilder() { MethodBody &body = m->body().indent(); auto scope = body.scope("return Base::getChecked(emitError, context", ");"); for (const auto ¶m : params) - body << ", " << param.getName(); + body << ", std::move(" << param.getName() << ")"; } static SmallVector From 060cea723d6b0f99caac1019a2521c5f3893d760 Mon Sep 17 00:00:00 2001 From: Alexandru Lorinti Date: Tue, 29 Jul 2025 18:13:03 +0300 Subject: [PATCH 2/3] Avoid copies in getChecked (#147721) Following-up on #68067 ; adding std::move to getChecked method as well. --- mlir/include/mlir/IR/StorageUniquerSupport.h | 2 +- mlir/test/lib/Dialect/Test/TestAttrDefs.td | 1 + mlir/test/lib/Dialect/Test/TestAttributes.cpp | 10 ++++++ mlir/test/mlir-tblgen/attrdefs.td | 5 +++ mlir/unittests/IR/AttributeTest.cpp | 31 ++++++++++++++++--- 5 files changed, 43 insertions(+), 6 deletions(-) diff --git a/mlir/include/mlir/IR/StorageUniquerSupport.h b/mlir/include/mlir/IR/StorageUniquerSupport.h index fb64f15162df..183d3f37ed3a 100644 --- a/mlir/include/mlir/IR/StorageUniquerSupport.h +++ b/mlir/include/mlir/IR/StorageUniquerSupport.h @@ -200,7 +200,7 @@ class StorageUserBase : public BaseT, public Traits... { // If the construction invariants fail then we return a null attribute. if (failed(ConcreteT::verify(emitErrorFn, args...))) return ConcreteT(); - return UniquerT::template get(ctx, args...); + return UniquerT::template get(ctx, std::forward(args)...); } /// Get an instance of the concrete type from a void pointer. diff --git a/mlir/test/lib/Dialect/Test/TestAttrDefs.td b/mlir/test/lib/Dialect/Test/TestAttrDefs.td index 2c5acb1b99a4..64ccd8b36c5b 100644 --- a/mlir/test/lib/Dialect/Test/TestAttrDefs.td +++ b/mlir/test/lib/Dialect/Test/TestAttrDefs.td @@ -331,6 +331,7 @@ def TestCopyCount : Test_Attr<"TestCopyCount"> { let mnemonic = "copy_count"; let parameters = (ins TestParamCopyCount:$copy_count); let assemblyFormat = "`<` $copy_count `>`"; + let genVerifyDecl = 1; } def TestConditionalAliasAttr : Test_Attr<"TestConditionalAlias"> { diff --git a/mlir/test/lib/Dialect/Test/TestAttributes.cpp b/mlir/test/lib/Dialect/Test/TestAttributes.cpp index 32fef18261ce..c2ea88bbe45c 100644 --- a/mlir/test/lib/Dialect/Test/TestAttributes.cpp +++ b/mlir/test/lib/Dialect/Test/TestAttributes.cpp @@ -177,6 +177,16 @@ static void printTrueFalse(AsmPrinter &p, std::optional result) { p << (*result ? "true" : "false"); } +//===----------------------------------------------------------------------===// +// TestCopyCountAttr Implementation +//===----------------------------------------------------------------------===// + +LogicalResult TestCopyCountAttr::verify( + llvm::function_ref<::mlir::InFlightDiagnostic()> /*emitError*/, + CopyCount /*copy_count*/) { + return success(); +} + //===----------------------------------------------------------------------===// // CopyCountAttr Implementation //===----------------------------------------------------------------------===// diff --git a/mlir/test/mlir-tblgen/attrdefs.td b/mlir/test/mlir-tblgen/attrdefs.td index 35d2c49619ee..8bd4af6ee73b 100644 --- a/mlir/test/mlir-tblgen/attrdefs.td +++ b/mlir/test/mlir-tblgen/attrdefs.td @@ -115,6 +115,11 @@ def B_CompoundAttrA : TestAttr<"CompoundA"> { // DEF: return new (allocator.allocate()) // DEF-SAME: CompoundAAttrStorage(std::move(widthOfSomething), std::move(exampleTdType), std::move(apFloat), std::move(dims), std::move(inner)); +// DEF: CompoundAAttr CompoundAAttr::getChecked( +// DEF-SAME: int widthOfSomething, ::test::SimpleTypeA exampleTdType, ::llvm::APFloat apFloat, ::llvm::ArrayRef dims, ::mlir::Type inner +// DEF-SAME: ) +// DEF-NEXT: return Base::getChecked(emitError, context, std::move(widthOfSomething), std::move(exampleTdType), std::move(apFloat), std::move(dims), std::move(inner)); + // DEF: ::mlir::Type CompoundAAttr::getInner() const { // DEF-NEXT: return getImpl()->inner; } diff --git a/mlir/unittests/IR/AttributeTest.cpp b/mlir/unittests/IR/AttributeTest.cpp index e72bfe9d82e7..86e744bd3930 100644 --- a/mlir/unittests/IR/AttributeTest.cpp +++ b/mlir/unittests/IR/AttributeTest.cpp @@ -462,8 +462,9 @@ TEST(SubElementTest, Nested) { {strAttr, trueAttr, falseAttr, boolArrayAttr, dictAttr})); } -// Test how many times we call copy-ctor when building an attribute. -TEST(CopyCountAttr, CopyCount) { +// Test how many times we call copy-ctor when building an attribute with the +// 'get' method. +TEST(CopyCountAttr, CopyCountGet) { MLIRContext context; context.loadDialect(); @@ -474,15 +475,35 @@ TEST(CopyCountAttr, CopyCount) { test::CopyCount::counter = 0; test::TestCopyCountAttr::get(&context, std::move(copyCount)); #ifndef NDEBUG - // One verification enabled only in assert-mode requires a copy. - EXPECT_EQ(counter1, 1); - EXPECT_EQ(test::CopyCount::counter, 1); + // One verification enabled only in assert-mode requires two copies: one for + // calling 'verifyInvariants' and one for calling 'verify' inside + // 'verifyInvariants'. + EXPECT_EQ(counter1, 2); + EXPECT_EQ(test::CopyCount::counter, 2); #else EXPECT_EQ(counter1, 0); EXPECT_EQ(test::CopyCount::counter, 0); #endif } +// Test how many times we call copy-ctor when building an attribute with the +// 'getChecked' method. +TEST(CopyCountAttr, CopyCountGetChecked) { + MLIRContext context; + context.loadDialect(); + test::CopyCount::counter = 0; + test::CopyCount copyCount("hello"); + auto loc = UnknownLoc::get(&context); + test::TestCopyCountAttr::getChecked(loc, &context, std::move(copyCount)); + int counter1 = test::CopyCount::counter; + test::CopyCount::counter = 0; + test::TestCopyCountAttr::getChecked(loc, &context, std::move(copyCount)); + // The verifiers require two copies: one for calling 'verifyInvariants' and + // one for calling 'verify' inside 'verifyInvariants'. + EXPECT_EQ(counter1, 2); + EXPECT_EQ(test::CopyCount::counter, 2); +} + // Test stripped printing using test dialect attribute. TEST(CopyCountAttr, PrintStripped) { MLIRContext context; From 55b2277eb20d770dc6cb061eae4128deb9cd8b1e Mon Sep 17 00:00:00 2001 From: Alexandru Lorinti Date: Tue, 5 Aug 2025 03:25:30 +0300 Subject: [PATCH 3/3] update EXPECT_EQ and comments to match current state of npu-plugin-llvm --- mlir/unittests/IR/AttributeTest.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/mlir/unittests/IR/AttributeTest.cpp b/mlir/unittests/IR/AttributeTest.cpp index 86e744bd3930..9f8abdb5096e 100644 --- a/mlir/unittests/IR/AttributeTest.cpp +++ b/mlir/unittests/IR/AttributeTest.cpp @@ -475,11 +475,9 @@ TEST(CopyCountAttr, CopyCountGet) { test::CopyCount::counter = 0; test::TestCopyCountAttr::get(&context, std::move(copyCount)); #ifndef NDEBUG - // One verification enabled only in assert-mode requires two copies: one for - // calling 'verifyInvariants' and one for calling 'verify' inside - // 'verifyInvariants'. - EXPECT_EQ(counter1, 2); - EXPECT_EQ(test::CopyCount::counter, 2); + // One verification enabled only in assert-mode requires a copy. + EXPECT_EQ(counter1, 1); + EXPECT_EQ(test::CopyCount::counter, 1); #else EXPECT_EQ(counter1, 0); EXPECT_EQ(test::CopyCount::counter, 0); @@ -498,10 +496,9 @@ TEST(CopyCountAttr, CopyCountGetChecked) { int counter1 = test::CopyCount::counter; test::CopyCount::counter = 0; test::TestCopyCountAttr::getChecked(loc, &context, std::move(copyCount)); - // The verifiers require two copies: one for calling 'verifyInvariants' and - // one for calling 'verify' inside 'verifyInvariants'. - EXPECT_EQ(counter1, 2); - EXPECT_EQ(test::CopyCount::counter, 2); + // One verification requires a copy. + EXPECT_EQ(counter1, 1); + EXPECT_EQ(test::CopyCount::counter, 1); } // Test stripped printing using test dialect attribute.