[clang-tidy] Add MLIR check for old op builder usage. (#149148)

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]>

Author: 3 people

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

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp

@@ -18,6 +18,7 @@
 #include "PreferRegisterOverUnsignedCheck.h"
 #include "PreferStaticOverAnonymousNamespaceCheck.h"
 #include "TwineLocalCheck.h"
+#include "UseNewMLIROpBuilderCheck.h"
 
 namespace clang::tidy {
 namespace llvm_check {
@@ -40,6 +41,8 @@ class LLVMModule : public ClangTidyModule {
     CheckFactories.registerCheck<readability::QualifiedAutoCheck>(
         "llvm-qualified-auto");
     CheckFactories.registerCheck<TwineLocalCheck>("llvm-twine-local");
+    CheckFactories.registerCheck<UseNewMlirOpBuilderCheck>(
+        "llvm-use-new-mlir-op-builder");
   }
 
   ClangTidyOptions getModuleOptions() override {

clang-tools-extra/clang-tidy/llvm/UseNewMLIROpBuilderCheck.cpp

@@ -0,0 +1,133 @@
+//===--- UseNewMLIROpBuilderCheck.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 "UseNewMLIROpBuilderCheck.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"
+#include "llvm/Support/FormatVariadic.h"
+
+namespace clang::tidy::llvm_check {
+namespace {
+
+using namespace ::clang::ast_matchers;
+using namespace ::clang::transformer;
+
+EditGenerator rewrite(RangeSelector Call, RangeSelector Builder,
+                      RangeSelector CallArgs) {
+  // This is using an EditGenerator rather than ASTEdit as we want to warn even
+  // if in macro.
+  return [Call = std::move(Call), Builder = std::move(Builder),
+          CallArgs =
+              std::move(CallArgs)](const MatchFinder::MatchResult &Result)
+             -> Expected<SmallVector<transformer::Edit, 1>> {
+    Expected<CharSourceRange> CallRange = Call(Result);
+    if (!CallRange)
+      return CallRange.takeError();
+    SourceManager &SM = *Result.SourceManager;
+    const LangOptions &LangOpts = Result.Context->getLangOpts();
+    SourceLocation Begin = CallRange->getBegin();
+
+    // This will result in just a warning and no edit.
+    bool InMacro = CallRange->getBegin().isMacroID();
+    if (InMacro) {
+      while (SM.isMacroArgExpansion(Begin))
+        Begin = SM.getImmediateExpansionRange(Begin).getBegin();
+      Edit WarnOnly;
+      WarnOnly.Kind = EditKind::Range;
+      WarnOnly.Range = CharSourceRange::getCharRange(Begin, Begin);
+      return SmallVector<Edit, 1>({WarnOnly});
+    }
+
+    // This will try to extract the template argument as written so that the
+    // rewritten code looks closest to original.
+    auto NextToken = [&](std::optional<Token> CurrentToken) {
+      if (!CurrentToken)
+        return CurrentToken;
+      if (CurrentToken->getEndLoc() >= CallRange->getEnd())
+        return std::optional<Token>();
+      return clang::Lexer::findNextToken(CurrentToken->getLocation(), SM,
+                                         LangOpts);
+    };
+    std::optional<Token> LessToken =
+        clang::Lexer::findNextToken(Begin, SM, LangOpts);
+    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 (std::optional<Token> 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");
+    }
+
+    Expected<CharSourceRange> BuilderRange = Builder(Result);
+    if (!BuilderRange)
+      return BuilderRange.takeError();
+    Expected<CharSourceRange> CallArgsRange = CallArgs(Result);
+    if (!CallArgsRange)
+      return CallArgsRange.takeError();
+
+    // Helper for concatting below.
+    auto GetText = [&](const CharSourceRange &Range) {
+      return clang::Lexer::getSourceText(Range, SM, LangOpts);
+    };
+
+    Edit Replace;
+    Replace.Kind = EditKind::Range;
+    Replace.Range = *CallRange;
+    std::string CallArgsStr;
+    // Only emit args if there are any.
+    if (auto CallArgsText = GetText(*CallArgsRange).ltrim();
+        !CallArgsText.rtrim().empty()) {
+      CallArgsStr = llvm::formatv(", {}", CallArgsText);
+    }
+    Replace.Replacement =
+        llvm::formatv("{}::create({}{})",
+                      GetText(CharSourceRange::getTokenRange(
+                          LessToken->getEndLoc(), EndToken->getLastLoc())),
+                      GetText(*BuilderRange), CallArgsStr);
+
+    return SmallVector<Edit, 1>({Replace});
+  };
+}
+
+RewriteRuleWith<std::string> useNewMlirOpBuilderCheckRule() {
+  return makeRule(
+      cxxMemberCallExpr(
+          on(expr(hasType(
+                      cxxRecordDecl(isSameOrDerivedFrom("::mlir::OpBuilder"))))
+                 .bind("builder")),
+          callee(cxxMethodDecl(hasTemplateArgument(0, templateArgument()))),
+          callee(cxxMethodDecl(hasName("create"))))
+          .bind("call"),
+      rewrite(node("call"), node("builder"), callArgs("call")),
+      cat("use 'OpType::create(builder, ...)' instead of "
+          "'builder.create<OpType>(...)'"));
+}
+} // namespace
+
+UseNewMlirOpBuilderCheck::UseNewMlirOpBuilderCheck(StringRef Name,
+                                                   ClangTidyContext *Context)
+    : TransformerClangTidyCheck(useNewMlirOpBuilderCheckRule(), Name, Context) {
+}
+
+} // namespace clang::tidy::llvm_check

clang-tools-extra/clang-tidy/llvm/UseNewMLIROpBuilderCheck.h

@@ -0,0 +1,29 @@
+//===--- UseNewMLIROpBuilderCheck.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_USENEWMLIROPBUILDERCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_USENEWMLIROPBUILDERCHECK_H
+
+#include "../utils/TransformerClangTidyCheck.h"
+
+namespace clang::tidy::llvm_check {
+
+/// Checks for uses of MLIR's old/to be deprecated `OpBuilder::create<T>` form
+/// and suggests using `T::create` instead.
+class UseNewMlirOpBuilderCheck : public utils::TransformerClangTidyCheck {
+public:
+  UseNewMlirOpBuilderCheck(StringRef Name, ClangTidyContext *Context);
+
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return getLangOpts().CPlusPlus;
+  }
+};
+
+} // namespace clang::tidy::llvm_check
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_LLVM_USENEWMLIROPBUILDERCHECK_H

clang-tools-extra/docs/ReleaseNotes.rst

@@ -96,6 +96,12 @@ Improvements to clang-tidy
 New checks
 ^^^^^^^^^^
 
+- New :doc:`llvm-mlir-op-builder
+  <clang-tidy/checks/llvm/use-new-mlir-op-builder>` check.
+
+  Checks for uses of MLIR's old/to be deprecated ``OpBuilder::create<T>`` form
+  and suggests using ``T::create`` instead.
+
 New check aliases
 ^^^^^^^^^^^^^^^^^
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

@@ -247,6 +247,7 @@ Clang-Tidy Checks
    :doc:`linuxkernel-must-check-errs <linuxkernel/must-check-errs>`,
    :doc:`llvm-header-guard <llvm/header-guard>`,
    :doc:`llvm-include-order <llvm/include-order>`, "Yes"
+   :doc:`llvm-use-new-mlir-op-builder <llvm/use-new-mlir-op-builder>`, "Yes"
    :doc:`llvm-namespace-comment <llvm/namespace-comment>`,
    :doc:`llvm-prefer-isa-or-dyn-cast-in-conditionals <llvm/prefer-isa-or-dyn-cast-in-conditionals>`, "Yes"
    :doc:`llvm-prefer-register-over-unsigned <llvm/prefer-register-over-unsigned>`, "Yes"

clang-tools-extra/docs/clang-tidy/checks/llvm/use-new-mlir-op-builder.rst

@@ -0,0 +1,21 @@
+.. title:: clang-tidy - llvm-mlir-op-builder
+
+llvm-mlir-op-builder
+====================
+
+Checks for uses of MLIR's old/to be deprecated ``OpBuilder::create<T>`` form
+and suggests using ``T::create`` instead.
+
+Example
+-------
+
+.. code-block:: c++
+
+  builder.create<FooOp>(builder.getUnknownLoc(), "baz");
+
+
+Transforms to:
+
+.. code-block:: c++
+
+  FooOp::create(builder, builder.getUnknownLoc(), "baz");

clang-tools-extra/test/clang-tidy/checkers/llvm/use-new-mlir-op-builder.cpp

@@ -0,0 +1,72 @@
+// RUN: %check_clang_tidy --match-partial-fixes %s llvm-use-new-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
+
+#define ASSIGN(A, B, C, D) C A = B.create<C>(B.getUnknownLoc(), D)
+
+template <typename T>
+void g(mlir::OpBuilder &b) {
+  // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use 'OpType::create(builder, ...)' instead of 'builder.create<OpType>(...)' [llvm-use-new-mlir-op-builder]
+  // CHECK-FIXES: T::create(b, b.getUnknownLoc(), "gaz")
+  b.create<T>(b.getUnknownLoc(), "gaz");
+}
+
+void f() {
+  mlir::OpBuilder builder;
+  // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use 'OpType::create(builder, ...)' instead of 'builder.create<OpType>(...)' [llvm-use-new-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>(...)' [llvm-use-new-mlir-op-builder]
+  // CHECK-FIXES: NamedOp::create(builder, builder.getUnknownLoc(), "baz")
+  builder.create<NamedOp>(builder.getUnknownLoc(), "baz");
+
+  // CHECK-MESSAGES: :[[@LINE+4]]:3: warning: use 'OpType::create(builder, ...)' instead of 'builder.create<OpType>(...)' [llvm-use-new-mlir-op-builder]
+  // CHECK-FIXES: NamedOp::create(builder,
+  // CHECK-FIXES:   builder.getUnknownLoc(),
+  // CHECK-FIXES:   "caz")
+  builder.
+   create<NamedOp>(
+     builder.getUnknownLoc(),
+     "caz");
+
+  // CHECK-MESSAGES: :[[@LINE+1]]:3: warning: use 'OpType::create(builder, ...)' instead of 'builder.create<OpType>(...)' [llvm-use-new-mlir-op-builder]
+  ASSIGN(op, builder, NamedOp, "daz");
+
+  g<NamedOp>(builder);
+
+  mlir::ImplicitLocOpBuilder ib;
+  // CHECK-MESSAGES: :[[@LINE+2]]:3: warning: use 'OpType::create(builder, ...)' instead of 'builder.create<OpType>(...)' [llvm-use-new-mlir-op-builder]
+  // CHECK-FIXES: mlir::ModuleOp::create(ib)
+  ib.create<mlir::ModuleOp>(   );
+}