Added support for HiFi build for ISS (simulator) under EXECUTORCH_BUILD_CADENCE cmake-config switch #3629
Conversation
…LD_CADENCE cmake-config switch.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/3629
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
Hi @cad-audio! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
facebook-github-bot
added
the
CLA Signed
CMakeLists.txt
Outdated
| @@ -443,7 +443,9 @@ target_link_options_shared_lib(executorch) | |||
| # Real integrations should supply their own YAML file that only lists the | |||
| # operators necessary for the models that will run. | |||
| # | |||
| if(NOT EXECUTORCH_BUILD_CADENCE) | |||
There was a problem hiding this comment.
Rather than guarding this by NOT EXECUTORCH_BUILD_CADENCE flags i think we should maybe add a flag to not build portable ops. e.g. BUILD_EXECUTORCH_PORTABLE_OPS which is on by default and for cadence builds it can be disabled explicitly.
There was a problem hiding this comment.
updated the PR as per this suggestion
CMakeLists.txt
Outdated
| @@ -417,7 +417,7 @@ target_link_libraries(executorch_no_prim_ops PRIVATE program_schema) | |||
| # Check if dl exists for this toolchain and only then link it. | |||
| find_library(DL_LIBRARY_EXISTS NAMES dl) | |||
| # Check if the library was found | |||
| if(DL_LIBRARY_EXISTS) | |||
| if(DL_LIBRARY_EXISTS AND NOT EXECUTORCH_BUILD_CADENCE) | |||
There was a problem hiding this comment.
Can you please elaborate on why this change was needed?
There was a problem hiding this comment.
find_library() command sets DL_LIBRARY_EXISTS (incorrectly ?) for Cadence HiFi core.
It then fails link-time with 'xt-ld: cannot find libdl' error. So added that extra check.
There was a problem hiding this comment.
Would love this to not bound to any specific backend though. Maybe we can have an option EXECUTORCH_USE_DL and you guys can set it to False.
There was a problem hiding this comment.
I agree, that would be better.
There was a problem hiding this comment.
updated the PR as per this suggestion
CMakeLists.txt
Outdated
| # | ||
| cmake_dependent_option( | ||
| BUILD_EXECUTORCH_PORTABLE_OPS "Build portable_ops library." ON | ||
| "NOT EXECUTORCH_BUILD_CADENCE" OFF |
There was a problem hiding this comment.
I don't think we need to tie these together right. I think in the build commands you have for Cadence you can just explicitly turn BUILD_EXECUTORCH_PORTABLE_OPS off? Same with EXECUTORCH_USE_DL.
|
@cad-audio thanks for addressing the comments. |
|
@mcremon-meta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@cad-audio still looking into it, but maybe a rebase would help to see if CI gets happier |
|
@mcremon-meta merged this pull request in 8f12da1. |
Differential Revision: D59294438
Summary: ^ Differential Revision: D59294438
…LD_CADENCE cmake-config switch.