-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[mlir][bazel] Export headers either from :Transforms or :TransformUtils #86819
Conversation
Split them according to their implementation. Ideally, header files should be used by only one target, but this is hard because CMake is less strict with headers (no layering check). But even with bazel, headers should only be exported once in the `hdrs` attribute. Other targets may use them in the `srcs` attribute to avoid circular dependencies.
hdrs = [ | ||
"include/mlir/Transforms/CFGToSCF.h", | ||
"include/mlir/Transforms/CommutativityUtils.h", | ||
"include/mlir/Transforms/ControlFlowSinkUtils.h", | ||
"include/mlir/Transforms/DialectConversion.h", | ||
"include/mlir/Transforms/FoldUtils.h", | ||
"include/mlir/Transforms/GreedyPatternRewriteDriver.h", | ||
"include/mlir/Transforms/Inliner.h", | ||
"include/mlir/Transforms/LoopInvariantCodeMotionUtils.h", | ||
"include/mlir/Transforms/OneToNTypeConversion.h", | ||
"include/mlir/Transforms/RegionUtils.h", | ||
"include/mlir/Transforms/TopologicalSortUtils.h", | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the .cpp files are in lib/Transforms/Utils, why are the corresponding headers in include/mlir/Transforms, and not include/mlir/Transforms/Utils? Ideally you could have this be glob(["include/mlir/Transforms/Utils/*.cpp"])
to match the srcs glob(["lib/Transforms/Utils/*.cpp"])
.
Is it feasible to move those files? How big of a change would that be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might make sense, but I don't really want to start another discussion about moving code around to better match the bazel build graph.
@@ -7851,7 +7887,16 @@ cc_library( | |||
srcs = glob([ | |||
"lib/Transforms/*.cpp", | |||
]), | |||
hdrs = glob(["include/mlir/Transforms/*.h"]), | |||
hdrs = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(likewise, it'd be nice if this could remain as glob(["include/mlir/Transforms/*.h"])
)
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 619443019
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 620006777
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 620006777
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 620006777
…on for llvm/llvm-project#86819. PiperOrigin-RevId: 620006777
IREE-side changes to adapt to MLIR changes: 1. `initializeOptions` changes to adapt to llvm/llvm-project#87289 2. `enableFastMathMode` removal: llvm/llvm-project#86578. 3. Bazel changes to adapt to llvm/llvm-project#86819 IREE-side fixes for preexisting bugs revealed by a MLIR change: 1. `mlp_tosa` test fix: the shapes were inconsistent, used to accidentally work, until MLIR started catching it since llvm/llvm-project#85798. See diagnostic in [87396](llvm/llvm-project#87396 (comment)). FYI @MaheshRavishankar. IREE-side fixes accidentally lumped into this: 1. The `iree_copts.cmake` change: It just happens that my bleeding-edge Clang was updated and started diagnosing some code relying on C++20 semantics. Filed #16946 as TODO. --------- Co-authored-by: Scott Todd <scott.todd0@gmail.com>
Split them according to their implementation.
Ideally, header files should be used by only one target, but this is hard because CMake is less strict with headers (no layering check). But even with bazel, headers should only be exported once in the
hdrs
attribute. Other targets may use them in thesrcs
attribute to avoid circular dependencies.