-
Couldn't load subscription status.
- Fork 700
Added support for --lib-name in manual kernel registration #12193
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?
Added support for --lib-name in manual kernel registration #12193
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/12193
Note: Links to docs will display an error until the docs builds have been completed. ❌ 87 New FailuresAs of commit d83a541 with merge base ef48cc2 ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
|
@pytorchbot label "release notes: none" |
|
Didn't find following labels among repository labels: Release Notes: ops & kernels |
|
Thanks for the change! Wdyt if we call the newly exposed symbol as register_kerlens_ to match the Apple frameworks product naming? |
|
Thank you for submitting PR! Let me take a look |
| kernel_index=kernel_index, | ||
| manual_registration=options.manual_registration, | ||
| add_exception_boundary=options.add_exception_boundary, | ||
| lib_name=options.lib_name, |
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.
Let's error out, if lib_name is specified but manual_registration is not enabled.
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.
Also can you please add a unit test here: https://github.com/pytorch/executorch/blob/main/codegen/test/test_executorch_gen.py
| ::executorch::runtime::register_kernels({kernels_to_register}); | ||
| if (success_with_kernel_reg != Error::Ok) { | ||
| ET_LOG(Error, "Failed register all kernels"); | ||
| ET_LOG(Error, "Failed to register ${lib_name} kernels"); |
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.
Would be good to print out the number of kernels, also __FILE__ for debugging
| ROOT_OPS # comma separated operator names to be selected | ||
| INCLUDE_ALL_OPS # boolean flag to include all operators | ||
| OPS_FROM_MODEL # path to a pte file of model to select operators from | ||
| DTYPE_SELECTIVE_BUILD # boolean flag to enable dtye selection |
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.
Typo. Thank you for adding this docstring!
tools/cmake/Codegen.cmake
Outdated
| if(GEN_LIB_NAME) | ||
| list(APPEND _gen_command --lib-name=${GEN_LIB_NAME}) | ||
| endif() |
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.
Ok this is a bit of problem.
We want to support 3 options:
- Using static initializer to register all kernels (default option)
- Using manual registration
2.1 Generate register_all_kernels() API
2.2 Generate register_{lib_name}_kernels() API
2.1 already exist and we don't want to break it. Now if you look at here, GEN_LIB_NAME is always being passed in, which effectively change 2.1 behavior to 2.2. Can we design the API in a way that we an support all 3 options?
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.
Please use register_kernels_{lib name} format :)
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.
Why does it matter?
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.
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.
Ok this is a bit of problem.
We want to support 3 options:
- Using static initializer to register all kernels (default option)
- Using manual registration
2.1 Generate register_all_kernels() API
2.2 Generate register_{lib_name}_kernels() API2.1 already exist and we don't want to break it. Now if you look at here,
GEN_LIB_NAMEis always being passed in, which effectively change 2.1 behavior to 2.2. Can we design the API in a way that we an support all 3 options?
So do you mean that I should also support the case where there is manual registration along with lib name then only I should pass GEN_LIB_NAME so this way it doesn't break already existing 2.1?
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.
So do you mean that I should also support the case where there is manual registration along with lib name
Yeah, you can imagine somewhere people are using manual registration and calling register_all_kernels() API, we should still allow them to do that. So maybe in this macro you can add another boolean argument like USE_LIB_NAME_IN_REGISTER or something similar. By default it's false and only if it's true, we generate register_kernels_{lib_name}() instead of register_all_kernels() API.
|
Thanks for the contribution! Could you rebase this PR on top of main? Looks like some changes from #12112 were also pulled in here :0 |
|
Can anyone help me resolve this conflict issue in documentation file ? Also have I pushed the changes correctly? Other than my changes there are others changes as well |
Hi @Tanish2101, can you try rebasing on top of main, and picking only your changes? |
b47f474 to
4e0bac5
Compare
Thanks @lucylq now the problem is resolved. 👍 |
|
Please check my changes now. |
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.
Looks good to me. Thank you for adding this. Can you make sure CI looks ok before merging
|
Can you resolve the conflict and I can help kicking off the CI |
This PR was reopened (likely due to being reverted), so your approval was removed. Please request another review.
I have already resolved those conflicts which occured in |
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 can take a look on Monday
Please let me know if there are any changes required. |
|
ok resolved the conflict. Waiting for CI signals before I merge this |
|
@Tanish2101 seeing this error: Can you take a look? To repro this, run: |
Ok, I will check. |
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |

Summary
This PR adds support for explicit function-based kernel registration in ExecuTorch through a new
--lib-nameflag in the code generation workflow. This allows each kernel library to generate aregister_<lib_name>_kernels()function instead of relying on static constructors and-force_load, which improves build portability especially for platforms like Xcode where-force_loadneeds to be used currently.Key changes:
Introduced
--lib-nameoption for--manual-registrationmode incodegen.Generates
register_<lib_name>_kernels()in RegisterKernels.cpp and corresponding headers.Extended documentation under
kernel-library-selective-build.mdto describe the new registration mechanism.Fixes #11221
@larryliu0820 @shoumikhin