Skip to content

release/20.x: [mlir][cmake] Fix build race condition in Pass Manager tests #125834

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

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

DavidSpickett
Copy link
Collaborator

@DavidSpickett DavidSpickett commented Feb 5, 2025

This cherry picks
[mlir] Fix build race condition in Pass Manager tests (d906da5)
to the 20.x release branch.

This addresses issues that started with
#123910, which is already on the 20.x branch.

Linaro noticed this on our flang dylib (shared library) build bot.

In file included from /home/tcwg-buildbot/worker/flang-aarch64-dylib/llvm-project/mlir/test/lib/Pass/TestPassManager.cpp:10:
/home/tcwg-buildbot/worker/flang-aarch64-dylib/llvm-project/mlir/test/lib/Pass/../Dialect/Test/TestOps.h:148:10: fatal error: 'TestOps.h.inc' file not found
148 | #include "TestOps.h.inc"
| ^~~~~~~~~~~~~~~

We have tested these changes on the buildbot for the last 2 days and had no problems.
Whereas before it was failing maybe 1 in 10 builds, enough that multiple people
in the community noticed it.

Reported in #124485.

@DavidSpickett DavidSpickett requested a review from mgorny February 5, 2025 10:46
@llvmbot llvmbot added the mlir label Feb 5, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 5, 2025

@llvm/pr-subscribers-mlir

Author: David Spickett (DavidSpickett)

Changes

This cherry picks
[mlir][cmake] Add missing MLIRTestDialect dependency (#125004) (72b73c9) and
[mlir] Fix build race condition in Pass Manager tests (d906da5) to the 20.x release branch.

Both of these are addressing issues that started with #123910, which is already on the 20.x branch.

In each case the cherry-picked changes correct problems that would otherwise be a race condition in certain builds. Linaro noticed this on our flang dylib (shared library) build bot.

Failures look like:
In file included from /home/tcwg-buildbot/worker/flang-aarch64-dylib/llvm-project/mlir/test/lib/Pass/TestPassManager.cpp:10: /home/tcwg-buildbot/worker/flang-aarch64-dylib/llvm-project/mlir/test/lib/Pass/../Dialect/Test/TestOps.h:148:10: fatal error: 'TestOps.h.inc' file not found
148 | #include "TestOps.h.inc"
| ^~~~~~~~~~~~~~~

We have tested these changes on the buildbot for the last 2 days and had no problems. Whereas before it was failing maybe 1 in 10 builds, enough that multiple people in the community noticed it.

Reported in #124485 and #124335.


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

2 Files Affected:

  • (modified) mlir/test/lib/IR/CMakeLists.txt (+4)
  • (modified) mlir/test/lib/Pass/CMakeLists.txt (+3-1)
diff --git a/mlir/test/lib/IR/CMakeLists.txt b/mlir/test/lib/IR/CMakeLists.txt
index e5416da70d50080..eeb9cf1e34fc8c0 100644
--- a/mlir/test/lib/IR/CMakeLists.txt
+++ b/mlir/test/lib/IR/CMakeLists.txt
@@ -27,7 +27,11 @@ add_mlir_library(MLIRTestIR
   TestVisitorsGeneric.cpp
 
   EXCLUDE_FROM_LIBMLIR
+
+  DEPENDS
+  MLIRTestDialect
   )
+
 mlir_target_link_libraries(MLIRTestIR PUBLIC
   MLIRPass
   MLIRBytecodeReader
diff --git a/mlir/test/lib/Pass/CMakeLists.txt b/mlir/test/lib/Pass/CMakeLists.txt
index 6698af86b8ae66d..c5d0bab8ec74946 100644
--- a/mlir/test/lib/Pass/CMakeLists.txt
+++ b/mlir/test/lib/Pass/CMakeLists.txt
@@ -10,12 +10,14 @@ add_mlir_library(MLIRTestPass
 
   ADDITIONAL_HEADER_DIRS
   ${MLIR_MAIN_INCLUDE_DIR}/mlir/Pass
+
+  LINK_LIBS PUBLIC
+  MLIRTestDialect
   )
 mlir_target_link_libraries(MLIRTestPass PUBLIC
   ${conversion_libs}
   MLIRIR
   MLIRPass
-  MLIRTestDialect
   )
 
 target_include_directories(MLIRTestPass

@DavidSpickett
Copy link
Collaborator Author

These would be clean cherry-picks but I put them into one commit so I could include the details of what it fixes. I hope that's ok.

@DavidSpickett
Copy link
Collaborator Author

Actually, I see @nikic doing more fixes. So I will wait for you to finish your work and we can coordinate what needs to go into 20.x.

@nikic
Copy link
Contributor

nikic commented Feb 5, 2025

Sorry, I didn't see this. I just submitted #125837 for the issues I ran into.

@DavidSpickett
Copy link
Collaborator Author

We are not duplicating any commits but there will be a conflict, I can avoid that by not including the change that you were fixing up.

Will update this shortly.

@DavidSpickett DavidSpickett changed the title [mlir][cmake] Add missing MLIRTestDialect dependencies [mlir][cmake] Fix build race condition in Pass Manager tests Feb 5, 2025
@DavidSpickett DavidSpickett removed the request for review from dcaballe February 5, 2025 13:19
@DavidSpickett
Copy link
Collaborator Author

Now only includes the commit that I authored, and will not conflict with your pull request.

@tstellar
Copy link
Collaborator

tstellar commented Feb 7, 2025

@nikic does this look OK?

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

@nikic nikic changed the title [mlir][cmake] Fix build race condition in Pass Manager tests release/20.x: [mlir][cmake] Fix build race condition in Pass Manager tests Feb 8, 2025
This cherry picks
[mlir] Fix build race condition in Pass Manager tests (d906da5)
to the 20.x release branch.

This addresses issues that started with
llvm#123910, which is already on the 20.x branch.

Linaro noticed this on our flang dylib (shared library) build bot.

In file included from /home/tcwg-buildbot/worker/flang-aarch64-dylib/llvm-project/mlir/test/lib/Pass/TestPassManager.cpp:10:
/home/tcwg-buildbot/worker/flang-aarch64-dylib/llvm-project/mlir/test/lib/Pass/../Dialect/Test/TestOps.h:148:10: fatal error: 'TestOps.h.inc' file not found
  148 | #include "TestOps.h.inc"
      |          ^~~~~~~~~~~~~~~

We have tested these changes on the buildbot for the last 2 days and had no problems.
Whereas before it was failing maybe 1 in 10 builds, enough that multiple people
in the community noticed it.

Reported in llvm#124485.
@tstellar tstellar merged commit 04d5513 into llvm:release/20.x Feb 10, 2025
7 of 10 checks passed
Copy link

@DavidSpickett (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

6 participants