llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-mlir 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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
