Skip to content

[CMake][MLIR] Adding dummy target to synchronize LinalgNamedStructuredOps.yamlgen #108547

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

Closed

Conversation

stefankoncarevic
Copy link
Contributor

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Sep 13, 2024

@llvm/pr-subscribers-mlir-linalg

Author: None (stefankoncarevic)

Changes

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

1 Files Affected:

  • (modified) mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt (+9-2)
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt b/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
index 289c0e4bbdaf68..dec4b1cc1cc464 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
@@ -3,6 +3,7 @@ function(add_linalg_ods_yaml_gen yaml_ast_file output_file)
   set(YAML_AST_SOURCE ${CMAKE_CURRENT_SOURCE_DIR}/${yaml_ast_file})
   set(GEN_ODS_FILE ${CMAKE_CURRENT_BINARY_DIR}/${output_file}.yamlgen.td)
   set(GEN_CPP_FILE ${CMAKE_CURRENT_BINARY_DIR}/${output_file}.yamlgen.cpp.inc)
+  set(DUMMY_FILE ${CMAKE_CURRENT_BINARY_DIR}/dummy)
   set_source_files_properties(
     ${GEN_ODS_FILE}
     PROPERTIES GENERATED TRUE)
@@ -17,14 +18,20 @@ function(add_linalg_ods_yaml_gen yaml_ast_file output_file)
     DEPENDS
     ${MLIR_LINALG_ODS_YAML_GEN_EXE}
     ${MLIR_LINALG_ODS_YAML_GEN_TARGET})
+  add_custom_command(
+    OUTPUT ${DUMMY_FILE}
+    COMMAND ${CMAKE_COMMAND} -E touch ${DUMMY_FILE}
+    DEPENDS
+    ${GEN_ODS_FILE} ${GEN_CPP_FILE}
+  )
   add_custom_target(
     MLIR${output_file}YamlIncGen
     DEPENDS
     ${MLIR_LINALG_ODS_YAML_GEN_EXE}
     ${MLIR_LINALG_ODS_YAML_GEN_TARGET}
-    ${GEN_ODS_FILE} ${GEN_CPP_FILE})
+    ${GEN_ODS_FILE} ${GEN_CPP_FILE} ${DUMMY_FILE})
   set_target_properties(MLIR${output_file}YamlIncGen PROPERTIES FOLDER "MLIR/Tablegenning")
-  list(APPEND LLVM_TARGET_DEPENDS ${GEN_ODS_FILE})
+  list(APPEND LLVM_TARGET_DEPENDS ${GEN_ODS_FILE} ${DUMMY_FILE})
   set(LLVM_TARGET_DEPENDS ${LLVM_TARGET_DEPENDS} PARENT_SCOPE)
 endfunction()
 

@llvmbot
Copy link
Member

llvmbot commented Sep 13, 2024

@llvm/pr-subscribers-mlir

Author: None (stefankoncarevic)

Changes

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

1 Files Affected:

  • (modified) mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt (+9-2)
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt b/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
index 289c0e4bbdaf68..dec4b1cc1cc464 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
+++ b/mlir/include/mlir/Dialect/Linalg/IR/CMakeLists.txt
@@ -3,6 +3,7 @@ function(add_linalg_ods_yaml_gen yaml_ast_file output_file)
   set(YAML_AST_SOURCE ${CMAKE_CURRENT_SOURCE_DIR}/${yaml_ast_file})
   set(GEN_ODS_FILE ${CMAKE_CURRENT_BINARY_DIR}/${output_file}.yamlgen.td)
   set(GEN_CPP_FILE ${CMAKE_CURRENT_BINARY_DIR}/${output_file}.yamlgen.cpp.inc)
+  set(DUMMY_FILE ${CMAKE_CURRENT_BINARY_DIR}/dummy)
   set_source_files_properties(
     ${GEN_ODS_FILE}
     PROPERTIES GENERATED TRUE)
@@ -17,14 +18,20 @@ function(add_linalg_ods_yaml_gen yaml_ast_file output_file)
     DEPENDS
     ${MLIR_LINALG_ODS_YAML_GEN_EXE}
     ${MLIR_LINALG_ODS_YAML_GEN_TARGET})
+  add_custom_command(
+    OUTPUT ${DUMMY_FILE}
+    COMMAND ${CMAKE_COMMAND} -E touch ${DUMMY_FILE}
+    DEPENDS
+    ${GEN_ODS_FILE} ${GEN_CPP_FILE}
+  )
   add_custom_target(
     MLIR${output_file}YamlIncGen
     DEPENDS
     ${MLIR_LINALG_ODS_YAML_GEN_EXE}
     ${MLIR_LINALG_ODS_YAML_GEN_TARGET}
-    ${GEN_ODS_FILE} ${GEN_CPP_FILE})
+    ${GEN_ODS_FILE} ${GEN_CPP_FILE} ${DUMMY_FILE})
   set_target_properties(MLIR${output_file}YamlIncGen PROPERTIES FOLDER "MLIR/Tablegenning")
-  list(APPEND LLVM_TARGET_DEPENDS ${GEN_ODS_FILE})
+  list(APPEND LLVM_TARGET_DEPENDS ${GEN_ODS_FILE} ${DUMMY_FILE})
   set(LLVM_TARGET_DEPENDS ${LLVM_TARGET_DEPENDS} PARENT_SCOPE)
 endfunction()
 

@stefankoncarevic
Copy link
Contributor Author

We have encountered a race condition and compilation failure in the LinalgStructuredOps.td file, which is triggered by tablegen parsing. The issue is related to a mal-formated construct in the LinalgStructuredOps.yamlgen.td file.

To address this issue, we have made changes to introduce a dummy target that enforces synchronization points during the generation process. This ensures that the necessary files are fully generated before proceeding with the subsequent steps.

For more details, please refer to the following ticket:
ticket: https://github.com/ROCm/rocMLIR-internal/issues/524
PR: ROCm/rocMLIR#669

We would appreciate if someone could review this PR and provide feedback. If deemed appropriate, you can consider merging this PR.

@rengolin
Copy link
Member

The issue you listed is private (I get 404), so not sure on the context.

We're working to deprecate OpDSL for the named ops (#104783), which will eventually cleanup all those issues.

@stefankoncarevic
Copy link
Contributor Author

Here is the new link related to the issue:
#56269
I hope this helps for better understanding the context.

@stefankoncarevic
Copy link
Contributor Author

I'll be closing the current PR and we'll wait for the completion of the other effort, which might help address our issues. Thank you for the update.

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

Successfully merging this pull request may close these issues.

3 participants