https://github.com/jpienaar created https://github.com/llvm/llvm-project/pull/149148
Moving towards new create method invocation, add check to flag old usage. >From 311e30ac75c7fcc0a13ad7908cc1eaa1f7cb1e07 Mon Sep 17 00:00:00 2001 From: Jacques Pienaar <jpien...@google.com> Date: Wed, 16 Jul 2025 17:37:53 +0000 Subject: [PATCH] [clang-tidy] Add MLIR check for old op builder usage. Moving towards new create method invocation, add check to flag off usage. --- clang-tools-extra/clang-tidy/CMakeLists.txt | 2 + .../clang-tidy/mlir/CMakeLists.txt | 26 +++++ .../clang-tidy/mlir/MLIRTidyModule.cpp | 39 +++++++ .../clang-tidy/mlir/OpBuilderCheck.cpp | 102 ++++++++++++++++++ .../clang-tidy/mlir/OpBuilderCheck.h | 21 ++++ .../clang-tidy/checkers/mlir/op_builder.cpp | 51 +++++++++ 6 files changed, 241 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/mlir/CMakeLists.txt create mode 100644 clang-tools-extra/clang-tidy/mlir/MLIRTidyModule.cpp create mode 100644 clang-tools-extra/clang-tidy/mlir/OpBuilderCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/mlir/OpBuilderCheck.h create mode 100644 clang-tools-extra/test/clang-tidy/checkers/mlir/op_builder.cpp diff --git a/clang-tools-extra/clang-tidy/CMakeLists.txt b/clang-tools-extra/clang-tidy/CMakeLists.txt index 93117cf1d6373..b89003bf6c926 100644 --- a/clang-tools-extra/clang-tidy/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/CMakeLists.txt @@ -66,6 +66,7 @@ add_subdirectory(linuxkernel) add_subdirectory(llvm) add_subdirectory(llvmlibc) add_subdirectory(misc) +add_subdirectory(mlir) add_subdirectory(modernize) if(CLANG_TIDY_ENABLE_STATIC_ANALYZER) add_subdirectory(mpi) @@ -93,6 +94,7 @@ set(ALL_CLANG_TIDY_CHECKS clangTidyLLVMModule clangTidyLLVMLibcModule clangTidyMiscModule + clangTidyMLIRModule clangTidyModernizeModule clangTidyObjCModule clangTidyOpenMPModule diff --git a/clang-tools-extra/clang-tidy/mlir/CMakeLists.txt b/clang-tools-extra/clang-tidy/mlir/CMakeLists.txt new file mode 100644 index 0000000000000..2d0c969082218 --- /dev/null +++ b/clang-tools-extra/clang-tidy/mlir/CMakeLists.txt @@ -0,0 +1,26 @@ +set(LLVM_LINK_COMPONENTS + FrontendOpenMP + Support + ) + +add_clang_library(clangTidyMLIRModule STATIC + NewOpBuilderCheck.cpp + + LINK_LIBS + clangTidy + clangTidyReadabilityModule + clangTidyUtils + + DEPENDS + omp_gen + ClangDriverOptions + ) + +clang_target_link_libraries(clangTidyMLIRModule + PRIVATE + clangAST + clangASTMatchers + clangBasic + clangLex + clangTooling + ) diff --git a/clang-tools-extra/clang-tidy/mlir/MLIRTidyModule.cpp b/clang-tools-extra/clang-tidy/mlir/MLIRTidyModule.cpp new file mode 100644 index 0000000000000..75e403f1fab09 --- /dev/null +++ b/clang-tools-extra/clang-tidy/mlir/MLIRTidyModule.cpp @@ -0,0 +1,39 @@ +//===--- MLIRTidyModule.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 "../ClangTidy.h" +#include "../ClangTidyModule.h" +#include "../ClangTidyModuleRegistry.h" +#include "OpBuilderCheck.h" + +namespace clang::tidy { +namespace mlir_check { + +class MLIRModule : public ClangTidyModule { +public: + void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck<OpBuilderCheck>("mlir-op-builder"); + } + + ClangTidyOptions getModuleOptions() override { + ClangTidyOptions Options; + return Options; + } +}; + +// Register the LLVMTidyModule using this statically initialized variable. +static ClangTidyModuleRegistry::Add<MLIRModule> X("mlir-module", + "Adds MLIR lint checks."); + +} // namespace mlir_check + +// This anchor is used to force the linker to link in the generated object file +// and thus register the MLIRModule. +volatile int MLIRModuleAnchorSource = 0; // NOLINT(misc-use-internal-linkage) + +} // namespace clang::tidy diff --git a/clang-tools-extra/clang-tidy/mlir/OpBuilderCheck.cpp b/clang-tools-extra/clang-tidy/mlir/OpBuilderCheck.cpp new file mode 100644 index 0000000000000..7521096d5b91d --- /dev/null +++ b/clang-tools-extra/clang-tidy/mlir/OpBuilderCheck.cpp @@ -0,0 +1,102 @@ +//===--- OpBuilderCheck.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 "OpBuilderCheck.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Basic/LLVM.h" +#include "clang/Lex/Lexer.h" +#include "clang/Tooling/Transformer/RangeSelector.h" +#include "clang/Tooling/Transformer/RewriteRule.h" +#include "clang/Tooling/Transformer/SourceCode.h" +#include "clang/Tooling/Transformer/Stencil.h" +#include "llvm/Support/Error.h" + +namespace clang::tidy::mlir_check { +namespace { + +using namespace ::clang::ast_matchers; // NOLINT: Too many names. +using namespace ::clang::transformer; // NOLINT: Too many names. + +class TypeAsWrittenStencil : public StencilInterface { +public: + explicit TypeAsWrittenStencil(std::string S) : id(std::move(S)) {} + std::string toString() const override { + return (llvm::Twine("TypeAsWritten(\"") + id + "\")").str(); + } + + llvm::Error eval(const MatchFinder::MatchResult &match, + std::string *result) const override { + auto n = node(id)(match); + if (!n) + return n.takeError(); + auto srcRange = n->getAsRange(); + if (srcRange.isInvalid()) { + return llvm::make_error<llvm::StringError>(llvm::errc::invalid_argument, + "srcRange is invalid"); + } + auto range = n->getTokenRange(srcRange); + auto nextToken = [&](std::optional<Token> token) { + if (!token) + return token; + return clang::Lexer::findNextToken(token->getLocation(), + *match.SourceManager, + match.Context->getLangOpts()); + }; + auto lessToken = clang::Lexer::findNextToken( + range.getBegin(), *match.SourceManager, match.Context->getLangOpts()); + while (lessToken && lessToken->getKind() != clang::tok::less) { + lessToken = nextToken(lessToken); + } + if (!lessToken) { + return llvm::make_error<llvm::StringError>(llvm::errc::invalid_argument, + "missing '<' token"); + } + std::optional<Token> endToken = nextToken(lessToken); + for (auto greaterToken = nextToken(endToken); + greaterToken && greaterToken->getKind() != clang::tok::greater; + greaterToken = nextToken(greaterToken)) { + endToken = greaterToken; + } + if (!endToken) { + return llvm::make_error<llvm::StringError>(llvm::errc::invalid_argument, + "missing '>' token"); + } + *result += clang::tooling::getText( + CharSourceRange::getTokenRange(lessToken->getEndLoc(), + endToken->getLastLoc()), + *match.Context); + return llvm::Error::success(); + } + std::string id; +}; + +Stencil typeAsWritten(StringRef Id) { + // Using this instead of `describe` so that we get the exact same spelling. + return std::make_shared<TypeAsWrittenStencil>(std::string(Id)); +} + +RewriteRuleWith<std::string> OpBuilderCheckRule() { + return makeRule( + cxxMemberCallExpr( + on(expr(hasType( + cxxRecordDecl(isSameOrDerivedFrom("::mlir::OpBuilder")))) + .bind("builder")), + callee(cxxMethodDecl(hasTemplateArgument(0, templateArgument()))), + callee(cxxMethodDecl(hasName("create")))) + .bind("call"), + changeTo(cat(typeAsWritten("call"), "::create(", node("builder"), ", ", + callArgs("call"), ")")), + cat("Use OpType::create(builder, ...) instead of " + "builder.create<OpType>(...)")); +} +} // namespace + +OpBuilderCheck::OpBuilderCheck(StringRef Name, ClangTidyContext *Context) + : TransformerClangTidyCheck(OpBuilderCheckRule(), Name, Context) {} + +} // namespace clang::tidy::mlir_check diff --git a/clang-tools-extra/clang-tidy/mlir/OpBuilderCheck.h b/clang-tools-extra/clang-tidy/mlir/OpBuilderCheck.h new file mode 100644 index 0000000000000..792ac7b782add --- /dev/null +++ b/clang-tools-extra/clang-tidy/mlir/OpBuilderCheck.h @@ -0,0 +1,21 @@ +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MLIR_OPBUILDERCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MLIR_OPBUILDERCHECK_H + +#include "../utils/TransformerClangTidyCheck.h" + +namespace clang::tidy::mlir_check { + +/// Checks for uses of `OpBuilder::create<T>` and suggests using `T::create` +/// instead. +class OpBuilderCheck : public utils::TransformerClangTidyCheck { +public: + OpBuilderCheck(StringRef Name, ClangTidyContext *Context); + + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return getLangOpts().CPlusPlus; + } +}; + +} // namespace clang::tidy::mlir_check + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MLIR_OPBUILDERCHECK_H diff --git a/clang-tools-extra/test/clang-tidy/checkers/mlir/op_builder.cpp b/clang-tools-extra/test/clang-tidy/checkers/mlir/op_builder.cpp new file mode 100644 index 0000000000000..bf60c665e86ad --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/mlir/op_builder.cpp @@ -0,0 +1,51 @@ +// RUN: %check_clang_tidy --match-partial-fixes %s mlir-op-builder %t + +namespace mlir { +class Location {}; +class OpBuilder { +public: + template <typename OpTy, typename... Args> + OpTy create(Location location, Args &&...args) { + return OpTy(args...); + } + Location getUnknownLoc() { return Location(); } +}; +class ImplicitLocOpBuilder : public OpBuilder { +public: + template <typename OpTy, typename... Args> + OpTy create(Args &&...args) { + return OpTy(args...); + } +}; +struct ModuleOp { + ModuleOp() {} + static ModuleOp create(OpBuilder &builder, Location location) { + return ModuleOp(); + } +}; +struct NamedOp { + NamedOp(const char* name) {} + static NamedOp create(OpBuilder &builder, Location location, const char* name) { + return NamedOp(name); + } +}; +} // namespace mlir + +void f() { + mlir::OpBuilder builder; + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: Use OpType::create(builder, ...) instead of builder.create<OpType>(...) [mlir-op-builder] + // CHECK-FIXES: mlir:: ModuleOp::create(builder, builder.getUnknownLoc()) + builder.create<mlir:: ModuleOp>(builder.getUnknownLoc()); + + using mlir::NamedOp; + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: Use OpType::create(builder, ...) instead of builder.create<OpType>(...) [mlir-op-builder] + // CHECK-FIXES: NamedOp::create(builder, builder.getUnknownLoc(), "baz") + builder.create<NamedOp>(builder.getUnknownLoc(), "baz"); + + mlir::ImplicitLocOpBuilder ib; + // Note: extra space in the case where there is no other arguments. Could be + // improved, but also clang-format will do that just post. + // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: Use OpType::create(builder, ...) instead of builder.create<OpType>(...) [mlir-op-builder] + // CHECK-FIXES: mlir::ModuleOp::create(ib ) + ib.create<mlir::ModuleOp>(); +} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
