[clang-tools-extra] [mlir] [clang-tidy][mlir] Add check for passing value type by reference. (PR #179882)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 5 00:19:58 PST 2026
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-tools-extra
Author: Jacques Pienaar (jpienaar)
<details>
<summary>Changes</summary>
This instance is effectively a wrapper around a pointer and should be passed by value.
There are a few existing such cases in MLIR upstream. Didn't fix along with this to avoid mixing discussions.
---
Full diff: https://github.com/llvm/llvm-project/pull/179882.diff
11 Files Affected:
- (added) clang-tools-extra/clang-tidy/llvm/AvoidPassingMlirOpAsRefCheck.cpp (+51)
- (added) clang-tools-extra/clang-tidy/llvm/AvoidPassingMlirOpAsRefCheck.h (+31)
- (modified) clang-tools-extra/clang-tidy/llvm/CMakeLists.txt (+1)
- (modified) clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp (+3)
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+5)
- (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1)
- (added) clang-tools-extra/docs/clang-tidy/checks/llvm/avoid-passing-mlir-op-as-ref.rst (+21)
- (added) clang-tools-extra/test/clang-tidy/checkers/llvm/avoid-passing-mlir-op-as-ref.cpp (+33)
- (modified) mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp (+2-2)
- (modified) mlir/lib/Dialect/SCF/Transforms/ParallelLoopFusion.cpp (+1-1)
- (modified) mlir/unittests/TableGen/OpBuildGen.cpp (+1-1)
``````````diff
diff --git a/clang-tools-extra/clang-tidy/llvm/AvoidPassingMlirOpAsRefCheck.cpp b/clang-tools-extra/clang-tidy/llvm/AvoidPassingMlirOpAsRefCheck.cpp
new file mode 100644
index 0000000000000..dc8d14632582a
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/llvm/AvoidPassingMlirOpAsRefCheck.cpp
@@ -0,0 +1,51 @@
+//===--- AvoidPassingMlirOpAsRefCheck.cpp - clang-tidy --------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "AvoidPassingMlirOpAsRefCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::llvm_check {
+
+void AvoidPassingMlirOpAsRefCheck::registerMatchers(MatchFinder *Finder) {
+ // Match parameters that are references to classes derived from mlir::Op.
+ Finder->addMatcher(
+ parmVarDecl(
+ hasType(qualType(
+ references(qualType(hasDeclaration(
+ cxxRecordDecl(isSameOrDerivedFrom("::mlir::Op"))
+ .bind("op_type")))),
+ // We want to avoid matching `Op *&` (reference to pointer to Op)
+ // which is not common for Op but possible.
+ unless(references(pointerType())))),
+ unless(isImplicit()),
+ unless(hasAncestor(cxxConstructorDecl(
+ anyOf(isCopyConstructor(), isMoveConstructor())))),
+ unless(hasAncestor(cxxMethodDecl(isCopyAssignmentOperator()))),
+ unless(hasAncestor(cxxMethodDecl(isMoveAssignmentOperator()))),
+ decl().bind("param")),
+ this);
+}
+
+void AvoidPassingMlirOpAsRefCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ const auto *Param = Result.Nodes.getNodeAs<ParmVarDecl>("param");
+ const auto *OpType = Result.Nodes.getNodeAs<CXXRecordDecl>("op_type");
+
+ // Exclude if we can't find definitions.
+ if (!Param || !OpType)
+ return;
+
+ diag(Param->getLocation(),
+ "MLIR Op class '%0' should be passed by value, not by reference")
+ << OpType->getName();
+}
+
+} // namespace clang::tidy::llvm_check
diff --git a/clang-tools-extra/clang-tidy/llvm/AvoidPassingMlirOpAsRefCheck.h b/clang-tools-extra/clang-tidy/llvm/AvoidPassingMlirOpAsRefCheck.h
new file mode 100644
index 0000000000000..ff9a1172f52d8
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/llvm/AvoidPassingMlirOpAsRefCheck.h
@@ -0,0 +1,31 @@
+//===--- AvoidPassingMlirOpAsRefCheck.h - clang-tidy ------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_AVOIDPASSINGMLIROPASREFCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_AVOIDPASSINGMLIROPASREFCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::llvm_check {
+
+/// Flags function parameters of a type derived from mlir::Op that are passed by
+/// reference.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/llvm/avoid-passing-mlir-op-as-ref.html
+class AvoidPassingMlirOpAsRefCheck : public ClangTidyCheck {
+public:
+ AvoidPassingMlirOpAsRefCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::llvm_check
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_AVOIDPASSINGMLIROPASREFCHECK_H
diff --git a/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt b/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
index a807f0ab65f87..919ebffd19ac8 100644
--- a/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
@@ -4,6 +4,7 @@ set(LLVM_LINK_COMPONENTS
)
add_clang_library(clangTidyLLVMModule STATIC
+ AvoidPassingMlirOpAsRefCheck.cpp
HeaderGuardCheck.cpp
IncludeOrderCheck.cpp
LLVMTidyModule.cpp
diff --git a/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp b/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
index c180574bdeed6..22a2d4a747c96 100644
--- a/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
@@ -11,6 +11,7 @@
#include "../readability/ElseAfterReturnCheck.h"
#include "../readability/NamespaceCommentCheck.h"
#include "../readability/QualifiedAutoCheck.h"
+#include "AvoidPassingMlirOpAsRefCheck.h"
#include "HeaderGuardCheck.h"
#include "IncludeOrderCheck.h"
#include "PreferIsaOrDynCastInConditionalsCheck.h"
@@ -48,6 +49,8 @@ class LLVMModule : public ClangTidyModule {
"llvm-type-switch-case-types");
CheckFactories.registerCheck<UseNewMlirOpBuilderCheck>(
"llvm-use-new-mlir-op-builder");
+ CheckFactories.registerCheck<AvoidPassingMlirOpAsRefCheck>(
+ "llvm-avoid-passing-mlir-op-as-ref");
CheckFactories.registerCheck<UseRangesCheck>("llvm-use-ranges");
CheckFactories.registerCheck<UseVectorUtilsCheck>("llvm-use-vector-utils");
}
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index ea80502735ede..ef62284e0e453 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -97,6 +97,11 @@ Improvements to clang-tidy
New checks
^^^^^^^^^^
+- New :doc:`avoid-passing-mlir-op-as-ref`
+ <clang-tidy/checks/llvm/avoid-passing-mlir-op-as-ref>` check.
+
+ Finds cases where ``mlir::Op`` derived classes are passed by reference.
+
- New :doc:`llvm-type-switch-case-types
<clang-tidy/checks/llvm/type-switch-case-types>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 0eabd9929dc39..db2ae177b8eee 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -245,6 +245,7 @@ Clang-Tidy Checks
:doc:`hicpp-no-assembler <hicpp/no-assembler>`,
:doc:`hicpp-signed-bitwise <hicpp/signed-bitwise>`,
:doc:`linuxkernel-must-check-errs <linuxkernel/must-check-errs>`,
+ :doc:`llvm-avoid-passing-mlir-op-as-ref <llvm/avoid-passing-mlir-op-as-ref>`,
:doc:`llvm-header-guard <llvm/header-guard>`,
:doc:`llvm-include-order <llvm/include-order>`, "Yes"
:doc:`llvm-namespace-comment <llvm/namespace-comment>`,
diff --git a/clang-tools-extra/docs/clang-tidy/checks/llvm/avoid-passing-mlir-op-as-ref.rst b/clang-tools-extra/docs/clang-tidy/checks/llvm/avoid-passing-mlir-op-as-ref.rst
new file mode 100644
index 0000000000000..882ab18c4ccd5
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/llvm/avoid-passing-mlir-op-as-ref.rst
@@ -0,0 +1,21 @@
+.. title:: clang-tidy - llvm-avoid-passing-mlir-op-as-ref
+
+llvm-avoid-passing-mlir-op-as-ref
+=================================
+
+Flags function parameters of a type derived from ``mlir::Op`` that are passed by
+reference. ``mlir::Op`` derived classes are lightweight wrappers around a pointer
+and should be passed by value.
+
+Example
+-------
+
+.. code-block:: c++
+
+ // Bad: passed by reference
+ void processOp(const MyOp &op);
+ void mutateOp(MyOp &op);
+
+ // Good: passed by value
+ void processOp(MyOp op);
+ void mutateOp(MyOp op);
diff --git a/clang-tools-extra/test/clang-tidy/checkers/llvm/avoid-passing-mlir-op-as-ref.cpp b/clang-tools-extra/test/clang-tidy/checkers/llvm/avoid-passing-mlir-op-as-ref.cpp
new file mode 100644
index 0000000000000..f76d6ebd3ce0e
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/llvm/avoid-passing-mlir-op-as-ref.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_clang_tidy %s llvm-avoid-passing-mlir-op-as-ref %t
+
+namespace mlir {
+template <typename ConcreteType, template <typename T> class... Traits>
+class Op {
+public:
+ // Minimal definition to satisfy isSameOrDerivedFrom
+};
+} // namespace mlir
+
+class MyOp : public mlir::Op<MyOp> {
+ using Op::Op;
+};
+class OtherClass {};
+
+// Should trigger warning
+void badFunction(const MyOp &op) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: MLIR Op class 'MyOp' should be passed by value, not by reference [llvm-avoid-passing-mlir-op-as-ref]
+}
+
+// Should trigger warning logic for non-const ref too
+void badFunctionMutable(MyOp &op) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: MLIR Op class 'MyOp' should be passed by value, not by reference [llvm-avoid-passing-mlir-op-as-ref]
+}
+
+// Good: passed by value
+void goodFunction(MyOp op) {}
+
+// Good: not an Op
+void otherFunction(const OtherClass &c) {}
+
+// Good: pointer to Op (not common nor necessarily good practice)
+void pointerFunction(MyOp *op) {}
diff --git a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
index c3916219d1c93..786073533bfb1 100644
--- a/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -1548,7 +1548,7 @@ verifyCopyprivateVarList(Operation *op, OperandRange copyprivateVars,
funcOp = llvmFuncOp;
auto getNumArguments = [&] {
- return std::visit([](auto &f) { return f.getNumArguments(); }, *funcOp);
+ return std::visit([](auto f) { return f.getNumArguments(); }, *funcOp);
};
auto getArgumentType = [&](unsigned i) {
@@ -2526,7 +2526,7 @@ void ParallelOp::build(OpBuilder &builder, OperationState &state,
}
template <typename OpType>
-static LogicalResult verifyPrivateVarList(OpType &op) {
+static LogicalResult verifyPrivateVarList(OpType op) {
auto privateVars = op.getPrivateVars();
auto privateSyms = op.getPrivateSymsAttr();
diff --git a/mlir/lib/Dialect/SCF/Transforms/ParallelLoopFusion.cpp b/mlir/lib/Dialect/SCF/Transforms/ParallelLoopFusion.cpp
index 4ea832177c4f9..45e1537c049a2 100644
--- a/mlir/lib/Dialect/SCF/Transforms/ParallelLoopFusion.cpp
+++ b/mlir/lib/Dialect/SCF/Transforms/ParallelLoopFusion.cpp
@@ -163,7 +163,7 @@ static bool isFusionLegal(ParallelOp firstPloop, ParallelOp secondPloop,
/// Prepends operations of firstPloop's body into secondPloop's body.
/// Updates secondPloop with new loop.
-static void fuseIfLegal(ParallelOp firstPloop, ParallelOp &secondPloop,
+static void fuseIfLegal(ParallelOp firstPloop, ParallelOp secondPloop,
OpBuilder builder,
llvm::function_ref<bool(Value, Value)> mayAlias) {
Block *block1 = firstPloop.getBody();
diff --git a/mlir/unittests/TableGen/OpBuildGen.cpp b/mlir/unittests/TableGen/OpBuildGen.cpp
index 09430336de994..5f275ae060cc0 100644
--- a/mlir/unittests/TableGen/OpBuildGen.cpp
+++ b/mlir/unittests/TableGen/OpBuildGen.cpp
@@ -46,7 +46,7 @@ class OpBuildGenTest : public ::testing::Test {
// Verify that `op` has the given set of result types, operands, and
// attributes.
template <typename OpTy>
- void verifyOp(OpTy &&concreteOp, std::vector<Type> resultTypes,
+ void verifyOp(OpTy concreteOp, std::vector<Type> resultTypes,
std::vector<Value> operands,
std::vector<NamedAttribute> attrs) {
ASSERT_NE(concreteOp, nullptr);
``````````
</details>
https://github.com/llvm/llvm-project/pull/179882
More information about the cfe-commits
mailing list