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

[MLIR] Remove unneeded LLVMDialect.h include in ControlFlowToSCF.cpp #113560

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

RoboTux
Copy link
Contributor

@RoboTux RoboTux commented Oct 24, 2024

This fixes the following failure when doing a clean build (in particular
no .ninja* lying around) of lib/libMLIRControlFlowToSCF.a only:

In file included from llvm/include/llvm/IR/Module.h:22,
                 from mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h:37,
                 from mlir/lib/Conversion/ControlFlowToSCF/ControlFlowToSCF.cpp:19
llvm/include/llvm/IR/Attributes.h:90:14: fatal error: llvm/IR/Attributes.inc: No such file or directory

@llvmbot llvmbot added the mlir label Oct 24, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 24, 2024

@llvm/pr-subscribers-mlir

Author: Thomas Preud'homme (RoboTux)

Changes

This fixes the following failure when doing a clean build (in particular
no .ninja* lying around) of lib/libMLIRControlFlowToSCF.a only:

In file included from llvm/include/llvm/IR/Module.h:22,
                 from mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h:37,
                 from mlir/lib/Conversion/ControlFlowToSCF/ControlFlowToSCF.cpp:19
llvm/include/llvm/IR/Attributes.h:90:14: fatal error: llvm/IR/Attributes.inc: No such file or directory

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

1 Files Affected:

  • (modified) mlir/lib/Conversion/ControlFlowToSCF/CMakeLists.txt (+1)
diff --git a/mlir/lib/Conversion/ControlFlowToSCF/CMakeLists.txt b/mlir/lib/Conversion/ControlFlowToSCF/CMakeLists.txt
index e2f1677b1d0695..9a3f2dfc3799fd 100644
--- a/mlir/lib/Conversion/ControlFlowToSCF/CMakeLists.txt
+++ b/mlir/lib/Conversion/ControlFlowToSCF/CMakeLists.txt
@@ -12,6 +12,7 @@ add_mlir_conversion_library(MLIRControlFlowToSCF
   MLIRArithDialect
   MLIRControlFlowDialect
   MLIRFuncDialect
+  MLIRLLVMDialect
   MLIRSCFDialect
   MLIRUBDialect
   MLIRPass

@zero9178
Copy link
Member

There shouldn't be any dependency on the LLVM dialect in that pass that I am aware of, so I'd much rather remove the include than add a false library dependency.

This seems to also be what was done in #104832 but was seemingly reverted for unrelated reasons.
Could you try removing the include instead?

@joker-eph
Copy link
Collaborator

The proper fix would be an interface I think, the commit you mention is using a string to work around, but that looks a bit like a hack to me.

@zero9178
Copy link
Member

The proper fix would be an interface I think, the commit you mention is using a string to work around, but that looks a bit like a hack to me.

I think you may be mixing the changes done to SCFToControlFlow in that PR with the ones in ControlFlowToSCF, where the PR only removes the include, nothing else.

Fully agree otherwise.

This fixes the following failure when doing a clean build (in particular
no .ninja* lying around) of lib/libMLIRControlFlowToSCF.a only:
```
In file included from llvm/include/llvm/IR/Module.h:22,
                 from mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h:37,
                 from mlir/lib/Conversion/ControlFlowToSCF/ControlFlowToSCF.cpp:19
llvm/include/llvm/IR/Attributes.h:90:14: fatal error: llvm/IR/Attributes.inc: No such file or directory
```
@RoboTux RoboTux force-pushed the add_missing_controlflow_to_scf_llvm_dep branch from 16dd901 to ee01150 Compare October 25, 2024 14:26
@RoboTux RoboTux changed the title [MLIR] Add missing MLIRLLVMDialect dep to MLIRControlFlowToSCF [MLIR] Remove unneeded LLVMDialect.h include in ControlFlowToSCF.cpp Oct 25, 2024
Copy link
Member

@zero9178 zero9178 left a comment

Choose a reason for hiding this comment

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

LGTM!

@RoboTux RoboTux merged commit bbc0e63 into llvm:main Oct 25, 2024
8 checks passed
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
…lvm#113560)

This fixes the following failure when doing a clean build (in particular
no .ninja* lying around) of lib/libMLIRControlFlowToSCF.a only:
```
In file included from llvm/include/llvm/IR/Module.h:22,
                 from mlir/include/mlir/Dialect/LLVMIR/LLVMDialect.h:37,
                 from mlir/lib/Conversion/ControlFlowToSCF/ControlFlowToSCF.cpp:19
llvm/include/llvm/IR/Attributes.h:90:14: fatal error: llvm/IR/Attributes.inc: No such file or directory
```
@RoboTux RoboTux deleted the add_missing_controlflow_to_scf_llvm_dep branch January 31, 2025 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants