-
Notifications
You must be signed in to change notification settings - Fork 15k
[flang] Fix build on different of cores from #164630 #164841
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
base: main
Are you sure you want to change the base?
Conversation
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Jean-Didier PAILLEUX (JDPailleux) ChangesNormally fix incorrect linking introduced in #161179 with build in parallel. Full diff: https://github.com/llvm/llvm-project/pull/164841.diff 1 Files Affected:
diff --git a/flang/lib/Optimizer/Dialect/MIF/CMakeLists.txt b/flang/lib/Optimizer/Dialect/MIF/CMakeLists.txt
index ed8463e9b0330..d53937ebb49d4 100644
--- a/flang/lib/Optimizer/Dialect/MIF/CMakeLists.txt
+++ b/flang/lib/Optimizer/Dialect/MIF/CMakeLists.txt
@@ -8,7 +8,6 @@ add_flang_library(MIFDialect
LINK_LIBS
FIRDialect
FIRDialectSupport
- FIRSupport
LINK_COMPONENTS
AsmParser
|
|
This PR looks weird (removing things rather than adding to the CMakefile), but somehow it made our CI green. Note that I had to test for the other issue alongside, so it wasn't the recommended 'change one thing at a time' approach. |
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.
Thank you for taking time for looking into this but I don't think this is the whole right fix yet. The error message in #164630 would indicate a missing dependency between whatever is including MIFDialect.h and the CMake task which is generating the table gen parts of that header (MIFDialect).
I had a quick look and I see that the header file is included from flang/include/flang/Optimizer/Support/InitFIR.h. FIRSupport has no dependency upon MIFDialect and so there is no guarantee that it will be built after MIFDialect.
I think removing the dependency from MIFDialect to FIRSupport makes the issue much less likely (because CMake won't be trying to build FIRSupport before it has built MIFDialect) but it doesn't guarantee that the build order will be correct in the future for all machines.
Beware that the dependency is created by the presence of the include in a header (InitFIR.h) and so every library that includes InitFIR.h will also depend upon MIFDialect. However, they should already have a dependency on FIRSupport so it should come through transitively once FIRSupport has the right dependency.
I hope that made sense. Please feel free to ask me questions here or on the flang slack if I can help at all :)
|
That was my conclusion. I was unable to reproduce the dependency issue on my side. |
Seems like flang suffers from a major design issue when it comes to specifying dependencies. |
Normally fix incorrect linking introduced in #161179 with build in parallel.