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

adds missing header, removes Bazel unnecessary dependency #110932

Merged
merged 3 commits into from
Oct 2, 2024

Conversation

cjdb
Copy link
Contributor

@cjdb cjdb commented Oct 2, 2024

No description provided.

@llvmbot llvmbot added mlir:affine mlir bazel "Peripheral" support tier build system: utils/bazel labels Oct 2, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 2, 2024

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-affine

Author: Christopher Di Bella (cjdb)

Changes

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

2 Files Affected:

  • (modified) mlir/include/mlir/Dialect/Affine/IR/ValueBoundsOpInterfaceImpl.h (+1)
  • (modified) utils/bazel/llvm-project-overlay/mlir/BUILD.bazel (+20-1)
diff --git a/mlir/include/mlir/Dialect/Affine/IR/ValueBoundsOpInterfaceImpl.h b/mlir/include/mlir/Dialect/Affine/IR/ValueBoundsOpInterfaceImpl.h
index 451c466fa0c950..51831cb17992ac 100644
--- a/mlir/include/mlir/Dialect/Affine/IR/ValueBoundsOpInterfaceImpl.h
+++ b/mlir/include/mlir/Dialect/Affine/IR/ValueBoundsOpInterfaceImpl.h
@@ -9,6 +9,7 @@
 #ifndef MLIR_DIALECT_AFFINE_IR_VALUEBOUNDSOPINTERFACEIMPL_H
 #define MLIR_DIALECT_AFFINE_IR_VALUEBOUNDSOPINTERFACEIMPL_H
 
+#include <cstdint>
 #include "mlir/Support/LLVM.h"
 
 namespace mlir {
diff --git a/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel b/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
index 8340e25352a355..596cecdf2c2ac2 100644
--- a/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/mlir/BUILD.bazel
@@ -3749,10 +3749,10 @@ cc_library(
         ":BytecodeOpInterface",
         ":DialectUtils",
         ":IR",
+        ":MaskableOpInterface",
         ":ShapedOpInterfaces",
         ":SideEffectInterfaces",
         ":VectorDialect",
-        ":VectorOpsIncGen",
         ":ViewLikeInterface",
         ":XeGPUEnumsIncGen",
         ":XeGPUIncGen",
@@ -11719,6 +11719,25 @@ gentbl_cc_library(
     ],
 )
 
+cc_library(
+    name = "VectorOps",
+    hdrs = [
+        "include/mlir/Dialect/Vector/IR/VectorAttributes.h.inc",
+        "include/mlir/Dialect/Vector/IR/VectorDialect.h.inc",
+        "include/mlir/Dialect/Vector/IR/VectorEnums.h.inc",
+        "include/mlir/Dialect/Vector/IR/VectorOps.h",
+    ],
+    includes = ["include"],
+    deps = [
+        ":ArithDialect",
+        ":BytecodeOpInterface",
+        ":MaskableOpInterface",
+        ":VectorAttributesIncGen",
+        ":VectorDialectIncGen",
+        ":VectorOpsIncGen",
+    ],
+)
+
 gentbl_cc_library(
     name = "VectorAttributesIncGen",
     tbl_outs = [

Copy link

github-actions bot commented Oct 2, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@rupprecht rupprecht left a comment

Choose a reason for hiding this comment

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

MLIR seems to be passing at trunk, at least on buildkite. What issue does this fix?

I would expect the cstdint to fix a CppHeaderCompile issue (being able to compile a standalone header), and adding the MaskableOpInterface would fix a layering check. Why is VectorOpsIncGen being removed though?

Otherwise, this kind of change seems fine. LGTM with a tiny bit of context added to the PR description.

@cjdb
Copy link
Contributor Author

cjdb commented Oct 2, 2024

MLIR seems to be passing at trunk, at least on buildkite. What issue does this fix?

I would expect the cstdint to fix a CppHeaderCompile issue (being able to compile a standalone header), and adding the MaskableOpInterface would fix a layering check. Why is VectorOpsIncGen being removed though?

Otherwise, this kind of change seems fine. LGTM with a tiny bit of context added to the PR description.

Bazel isn't happy without this change. Specifically:

$ bazel build --config=generic_clang @llvm-project//mlir/test/Analysis:DataFlow/test-dead-code-analysis.mlir.test
INFO: Analyzed target @@llvm-project//mlir/test/Analysis:DataFlow/test-dead-code-analysis.mlir.test (1 packages loaded, 4265 targets configured).
ERROR: /home/cjdb/.cache/bazel/_bazel_cjdb/7630592ec0cf69943691bbf5cc2fc71f/external/llvm-project/mlir/BUILD.bazel:3913:11: Compiling mlir/lib/Dialect/Affine/IR/ValueBoundsOpInterfaceImpl.cpp failed: (Exit 1): clang-20 failed: error executing CppCompile command (from target @@llvm-project//mlir:AffineDialect) /home/cjdb/opt/bin/clang-20 -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics ... (remaining 124 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
In file included from external/llvm-project/mlir/lib/Dialect/Affine/IR/ValueBoundsOpInterfaceImpl.cpp:9:
external/llvm-project/mlir/include/mlir/Dialect/Affine/IR/ValueBoundsOpInterfaceImpl.h:31:11: error: unknown type name 'int64_t'; did you mean '__int64_t'?
   31 | FailureOr<int64_t> fullyComposeAndComputeConstantDelta(Value value1,
      |           ^
/usr/include/x86_64-linux-gnu/bits/types.h:44:25: note: '__int64_t' declared here
   44 | typedef signed long int __int64_t;
      |                         ^
1 error generated.
Target @@llvm-project//mlir/test/Analysis:DataFlow/test-dead-code-analysis.mlir.test failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 6.603s, Critical Path: 3.73s
INFO: 17 processes: 13 internal, 4 linux-sandbox.
ERROR: Build did NOT complete successfully

@cjdb cjdb merged commit 41eb186 into llvm:main Oct 2, 2024
4 of 6 checks passed
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
The missing header is necessary to keep building with Bazel happy, and the dependency was erroneously added.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel mlir:affine mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants