-
Notifications
You must be signed in to change notification settings - Fork 603
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
Add a dedicated iree_c_module CMake module #5214
Conversation
# TRANSLATE_TOOL: Translation tool to invoke (CMake target). | ||
# FLAGS: Flags to pass to the translation tool (list of strings). |
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.
Can you document the defaults here?
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.
Done. Also added those to iree_bytecode_module
.
set(_ARGS "${_FLAGS}") | ||
list(APPEND _ARGS "${CMAKE_CURRENT_SOURCE_DIR}/${_RULE_SRC}") | ||
list(APPEND _ARGS "-o") | ||
list(APPEND _ARGS "${_RULE_H_FILE_OUTPUT}") | ||
|
||
add_custom_command( | ||
OUTPUT "${_RULE_H_FILE_OUTPUT}" | ||
COMMAND ${_TRANSLATE_TOOL_EXECUTABLE} ${_ARGS} | ||
# Changes to either the translation tool or the input source should | ||
# trigger rebuilding. | ||
DEPENDS ${_TRANSLATE_TOOL_EXECUTABLE} ${_RULE_SRC} | ||
) |
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.
Seems like it would be just as clear to include the command args in the call to add_custom_command
set(_ARGS "${_FLAGS}") | |
list(APPEND _ARGS "${CMAKE_CURRENT_SOURCE_DIR}/${_RULE_SRC}") | |
list(APPEND _ARGS "-o") | |
list(APPEND _ARGS "${_RULE_H_FILE_OUTPUT}") | |
add_custom_command( | |
OUTPUT "${_RULE_H_FILE_OUTPUT}" | |
COMMAND ${_TRANSLATE_TOOL_EXECUTABLE} ${_ARGS} | |
# Changes to either the translation tool or the input source should | |
# trigger rebuilding. | |
DEPENDS ${_TRANSLATE_TOOL_EXECUTABLE} ${_RULE_SRC} | |
) | |
add_custom_command( | |
OUTPUT "${_RULE_H_FILE_OUTPUT}" | |
COMMAND | |
${_TRANSLATE_TOOL_EXECUTABLE} | |
${_FLAGS} | |
"${CMAKE_CURRENT_SOURCE_DIR}/${_RULE_SRC}" | |
"-o" "${_RULE_H_FILE_OUTPUT}" | |
# Changes to either the translation tool or the input source should | |
# trigger rebuilding. | |
DEPENDS ${_TRANSLATE_TOOL_EXECUTABLE} ${_RULE_SRC} | |
) |
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.
This is nearly a takeover from https://github.com/google/iree/blob/881de81238971869d6d62cabf8dc0b54a17be704/build_tools/cmake/iree_bytecode_module.cmake#L64%C3%9FL77.
I have no preference, but I think it might be nice to keep it consistent?
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.
Fair, though I don't think consistency in this case really buys us much. I don't care too much either way.
add_custom_target( | ||
${_GEN_TARGET} | ||
DEPENDS | ||
${_RULE_H_FILE_OUTPUT} | ||
) |
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.
I think that if the only thing that depends on a generated file from add_custom_command
is a library defined in the same CMakeLists.txt, you can just directly depend on it from that library.
This blog post is from 2015, but I think may still be relevant https://samthursfield.wordpress.com/2015/11/21/cmake-dependencies-between-targets-and-files-and-custom-commands/
I think that interface libraries do support sources (although the documentation is somewhat confused). https://cmake.org/cmake/help/v3.13/command/add_library.html#interface-libraries. We don't appear to use that functionality in iree_cc_library, but I think we maybe should.
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.
Discussed in discord. What we have here works and other things appear not to. The extra wrinkle of this being a generated header file (and therefore not compiled), means that my suggestion here doesn't work.
Instead of using `iree_bytecode_module` to generate a C module, this adds a dedicated `iree_c_module` CMake module.
* da64c93 Integrate MLIR-EmitC at iml130/mlir-emitc@dde739f (#5228) * a8d6c2a Fix doc publication by escaping our IR snippets in markdown. (#5227) * bd5e535 Introduce RVV VLS code-gen (#5199) * d496bc9 Avoid allocating temporary buffer for tensors derived from read-only tensors (.. * 1a93557 Tidying up a few pages of documentation. (#5225) * 46e8331 Add a dedicated iree_c_module CMake module (#5214) * 603b208 Merge google -> main (#5219) * c9f3742 Add support for control flow lowering in the VM to emitc target (#5208) * c8a7b2f Sort descriptors as expected by the native module (#5207) * 47183fb Revise compiler tracing to enable statistics aggregation. (#5218) * 500ec7b Set author to match committer for llvm submodule update action. (#5217) * 881de81 Generate and use the iree_hal_executable_library_t metadata. (#5195) * cf8180e Increase K tile size for small matrices. (#5213) * f5804ec Fix problem bug in MobileBert with vectorization enable. (#5211) COPYBARA_INTEGRATE_REVIEW=#5229 from ScottTodd:main-to-google da64c93 PiperOrigin-RevId: 365076337
* da64c93 Integrate MLIR-EmitC at iml130/mlir-emitc@dde739f (#5228) * a8d6c2a Fix doc publication by escaping our IR snippets in markdown. (#5227) * bd5e535 Introduce RVV VLS code-gen (#5199) * d496bc9 Avoid allocating temporary buffer for tensors derived from read-only tensors (.. * 1a93557 Tidying up a few pages of documentation. (#5225) * 46e8331 Add a dedicated iree_c_module CMake module (#5214) * 603b208 Merge google -> main (#5219) * c9f3742 Add support for control flow lowering in the VM to emitc target (#5208) * c8a7b2f Sort descriptors as expected by the native module (#5207) * 47183fb Revise compiler tracing to enable statistics aggregation. (#5218) * 500ec7b Set author to match committer for llvm submodule update action. (#5217) * 881de81 Generate and use the iree_hal_executable_library_t metadata. (#5195) * cf8180e Increase K tile size for small matrices. (#5213) * f5804ec Fix problem bug in MobileBert with vectorization enable. (#5211) PiperOrigin-RevId: 365076337
Instead of using
iree_bytecode_module
to generate a C module, thisadds a dedicated
iree_c_module
CMake module.