-
Notifications
You must be signed in to change notification settings - Fork 14.6k
[clang-tidy] Add MLIR check for old op builder usage. #149148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
b24a881
to
f5d8059
Compare
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Jacques Pienaar (jpienaar) ChangesMoving towards new create method invocation, add check to flag old usage. Full diff: https://github.com/llvm/llvm-project/pull/149148.diff 10 Files Affected:
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/ClangTidyForceLinker.h b/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
index adde9136ff1dd..3cde93124c6e4 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
@@ -94,6 +94,11 @@ extern volatile int MiscModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED MiscModuleAnchorDestination =
MiscModuleAnchorSource;
+// This anchor is used to force the linker to link the MLIRModule.
+extern volatile int MLIRModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED MLIRModuleAnchorDestination =
+ MLIRModuleAnchorSource;
+
// This anchor is used to force the linker to link the ModernizeModule.
extern volatile int ModernizeModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED ModernizeModuleAnchorDestination =
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..7d0b2de1df24c
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/mlir/CMakeLists.txt
@@ -0,0 +1,28 @@
+set(LLVM_LINK_COMPONENTS
+ FrontendOpenMP
+ Support
+ )
+
+add_clang_library(clangTidyMLIRModule STATIC
+ MLIRTidyModule.cpp
+ OpBuilderCheck.cpp
+
+ LINK_LIBS
+ clangTidy
+ clangTidyReadabilityModule
+ clangTidyUtils
+ clangTransformer
+
+ 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..f542020a0afdd
--- /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 ModuleModule 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/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 9eb3835fe8340..466b2a9a7e84c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -162,6 +162,11 @@ New checks
Finds uses of ``std::lock_guard`` and suggests replacing them with C++17's
alternative ``std::scoped_lock``.
+- New :doc:`mlir-op-builder
+ <clang-tidy/checks/mlir/op-builder>` check.
+
+ Flags usage of old OpBuilder format.
+
- New :doc:`portability-avoid-pragma-once
<clang-tidy/checks/portability/avoid-pragma-once>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 0cffbd323caa2..49cd008e7588c 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -24,6 +24,7 @@ Clang-Tidy Checks
llvm/*
llvmlibc/*
misc/*
+ mlir/*
modernize/*
mpi/*
objc/*
@@ -279,6 +280,7 @@ Clang-Tidy Checks
:doc:`misc-unused-using-decls <misc/unused-using-decls>`, "Yes"
:doc:`misc-use-anonymous-namespace <misc/use-anonymous-namespace>`,
:doc:`misc-use-internal-linkage <misc/use-internal-linkage>`, "Yes"
+ :doc:`mlir-op-builder <mlir/op-builder>`, "Yes"
:doc:`modernize-avoid-bind <modernize/avoid-bind>`, "Yes"
:doc:`modernize-avoid-c-arrays <modernize/avoid-c-arrays>`,
:doc:`modernize-concat-nested-namespaces <modernize/concat-nested-namespaces>`, "Yes"
diff --git a/clang-tools-extra/docs/clang-tidy/checks/mlir/op-builder.rst b/clang-tools-extra/docs/clang-tidy/checks/mlir/op-builder.rst
new file mode 100644
index 0000000000000..30bae06a36836
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/mlir/op-builder.rst
@@ -0,0 +1,22 @@
+.. title:: clang-tidy - mlir-op-builder
+
+mlir-op-builder
+===============
+
+Flags usage of old form of invoking create on `OpBuilder` and suggesting new
+form.
+
+Example
+-------
+
+.. code-block:: c++
+
+ builder.create<FooOp>(builder.getUnknownLoc(), "baz");
+
+
+Transforms to:
+
+.. code-block:: c++
+
+ FooOp::create(builder, builder.getUnknownLoc(), "baz");
+
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>();
+}
|
I think we should go through an RFC before creating a new module. TBH, I don't think we need a new module for MLIR in upstream since it is not a widely known thing in C++ community. Would it be viable for MLIR folks to use plugins or have a dedicated directory with custom checks in MLIR project like libcxx do. |
I considered placing it in LLVM module instead given part of LLVM project. |
I agree that a new module seems unnecessary, do we know that it will contain more checks in the future? If not moving to llvm or plugins would perhaps be more suitable. |
Good question. I think there are a few style things we could flag, but nothing planned. And this current one I'd assume lives for like 6-9 months and then we remove it. Having this flagged on commits to both MLIR & Flang to reduce migration need later. Well I know a couple of downstream projects that would want this too and I'd mass apply this across multiple projects while guarding against new additions before deprecation. I can move to LLVM module and then delete it in like 9 months. |
clang-tools-extra/docs/clang-tidy/checks/llvm/mlir-op-builder.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/llvm/mlir-op-builder.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/llvm/mlir-op-builder.rst
Outdated
Show resolved
Hide resolved
Moving towards new create method invocation, add check to flag old usage.
…lanned rewrites & limited scope
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/39212 Here is the relevant piece of the build log for the reference
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/33735 Here is the relevant piece of the build log for the reference
|
@EugeneZelenko i am also seeing downstream failures same as above noted by buildbots, could you revert and fix ? |
Hi, could you share build logs/link if you can? First fail seems unrelated to the PR. On second I don't see exactly that this new check fails buildbot, seems more like a general failure of build-node. |
Suppressed 5449 warnings (5449 in non-user code). |
Tons of failures on Gentoo as well:
At a quick glance, they look like something's segfaulting on exit:
|
Those failures don't seem related (no StringSet used here, not activated for abseil checks). If you can share longer build log or local repro instructions I can look. But this check should not be active on those many tests. AFAIK all the clang-tidy unit tests just enable one check at a time, so it shouldn't even be running during the abseil test. |
Well, that's what bisect said for me. 889faab passed, 3feb6f9 failed. llvm: llvm-core:llvm-22.0.0.9999:20250726-132422.log.gz (195k → 4.4M unpacked) |
Oh, I think I know what's wrong here. diff --git a/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt b/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
index 3232f6e2cafe..4f1da43d3f1b 100644
--- a/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
@@ -11,11 +11,13 @@ add_clang_library(clangTidyLLVMModule STATIC
PreferRegisterOverUnsignedCheck.cpp
PreferStaticOverAnonymousNamespaceCheck.cpp
TwineLocalCheck.cpp
+ UseNewMLIROpBuilderCheck.cpp
LINK_LIBS
clangTidy
clangTidyReadabilityModule
clangTidyUtils
+ clangTransformer
DEPENDS
omp_gen My guess is that you're bypassing the dylib, and effectively the executables end up linking both dylib and all static libraries, which is a recipe for disaster. |
(I'm trying a patch, will send a PR in a minute if it works) |
Yeah I was wondering if linkage related as that's only way with the code not running that it could have impact. Definitely not intentionally skipping it ... Well and it also explains why I couldn't see anything different while trying different sanitizers, as I wasn't doing any shared lib builds. |
Fix the regression introduced in llvm#149148 that incorrectly explicitly linked `clangTransformer` when dylib was used. As a result, the executables linking to `clangTidyLLVMModule` would end up linking both the dylib and a number of static clang libraries, leading to complete mayhem and undecipherable segmentation faults. Signed-off-by: Michał Górny <[email protected]>
Confirmed that #150769 fixes the issue. I'd appreciate if you could review it promptly, so I could do a fresh clean build and check for more issues today. |
Thanks! Done, I assume you can submit once it passes? Else, let me know. |
Yeah, I can. |
Fix the regression introduced in #149148 that incorrectly explicitly linked `clangTransformer` when dylib was used. As a result, the executables linking to `clangTidyLLVMModule` would end up linking both the dylib and a number of static clang libraries, leading to complete mayhem and undecipherable segmentation faults. Signed-off-by: Michał Górny <[email protected]>
Upstream is moving towards new create method invocation, add check to flag old usage that will be deprecated. --------- Co-authored-by: Baranov Victor <[email protected]> Co-authored-by: EugeneZelenko <[email protected]>
Fix the regression introduced in llvm#149148 that incorrectly explicitly linked `clangTransformer` when dylib was used. As a result, the executables linking to `clangTidyLLVMModule` would end up linking both the dylib and a number of static clang libraries, leading to complete mayhem and undecipherable segmentation faults. Signed-off-by: Michał Górny <[email protected]>
…llvm#149148)"" This reverts commit 5845e1a.
…llvm#149148)"" This reverts commit 5845e1a.
Moving towards new create method invocation, add check to flag old usage.