Skip to content
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

Disable clang-tidy misc-include-cleaner #83945

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

joker-eph
Copy link
Collaborator

This does not apply well to LLVM which intentionally rely on forward declarations. Also depending on the config flags passed to CMake the result can be different.

@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Mar 5, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 5, 2024

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Mehdi Amini (joker-eph)

Changes

This does not apply well to LLVM which intentionally rely on forward declarations. Also depending on the config flags passed to CMake the result can be different.


Full diff: https://github.com/llvm/llvm-project/pull/83945.diff

3 Files Affected:

  • (modified) .clang-tidy (+1-1)
  • (modified) mlir/include/mlir/IR/Matchers.h (+11)
  • (modified) mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp (+1-2)
diff --git a/.clang-tidy b/.clang-tidy
index 4e1cb114f43b2c..9cece0de812b8e 100644
--- a/.clang-tidy
+++ b/.clang-tidy
@@ -1,4 +1,4 @@
-Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-const-correctness,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion,-misc-use-anonymous-namespace,readability-identifier-naming'
+Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-const-correctness,-misc-unused-parameters,-misc-non-private-member-variables-in-classes,-misc-no-recursion,-misc-use-anonymous-namespace,readability-identifier-naming,-misc-include-cleaner'
 CheckOptions:
   - key:             readability-identifier-naming.ClassCase
     value:           CamelCase
diff --git a/mlir/include/mlir/IR/Matchers.h b/mlir/include/mlir/IR/Matchers.h
index f6417f62d09e8c..31a687dd422f60 100644
--- a/mlir/include/mlir/IR/Matchers.h
+++ b/mlir/include/mlir/IR/Matchers.h
@@ -15,9 +15,11 @@
 #ifndef MLIR_IR_MATCHERS_H
 #define MLIR_IR_MATCHERS_H
 
+#include "mlir/IR/BuiltinAttributeInterfaces.h"
 #include "mlir/IR/BuiltinAttributes.h"
 #include "mlir/IR/BuiltinTypes.h"
 #include "mlir/IR/OpDefinition.h"
+#include "llvm/Support/Casting.h"
 
 namespace mlir {
 
@@ -92,6 +94,15 @@ struct constant_op_binder {
     assert(succeeded(result) && "expected ConstantLike op to be foldable");
 
     if (auto attr = llvm::dyn_cast<AttrT>(foldedOp.front().get<Attribute>())) {
+      // Check that the attribute type matches the result type, folder shouldn't do
+      // this and maybe we should assert here, unfortunately llvm.constant accepts
+      // at the moment index attributes when the SSA type is integer.
+      if (auto typedAttr = dyn_cast<TypedAttr>(attr)) {
+        if (typedAttr.getType() != op->getResult(0).getType()) {
+          llvm::errs() << "Type mismatch: " << attr << "  " << *op << "\n";
+          return false;
+        }
+      }
       if (bind_value)
         *bind_value = attr;
       return true;
diff --git a/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp b/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
index 51d2f5e01b7235..665536fb0c85ee 100644
--- a/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
+++ b/mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
@@ -809,8 +809,7 @@ LogicalResult RegionPatternRewriteDriver::simplify(bool *changed) && {
       // accidentally reversing the constant order during processing.
       Attribute constValue;
       if (matchPattern(op, m_Constant(&constValue)))
-        if (!folder.insertKnownConstant(op, constValue))
-          return true;
+        return !folder.insertKnownConstant(op, constValue);
       return false;
     };
 

This does not apply well to LLVM which intentionally rely on forward declarations.
Also depending on the config flags passed to CMake the result can be different.
@joker-eph joker-eph force-pushed the clang-tidy-misc-cleaner branch from 9059be8 to d3f7235 Compare March 5, 2024 02:13
Copy link
Member

@ftynse ftynse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@joker-eph joker-eph merged commit 64faa52 into llvm:main Mar 5, 2024
4 checks passed
@joker-eph joker-eph deleted the clang-tidy-misc-cleaner branch March 5, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants