Added support for --lib-name in manual kernel registration#12193
Added support for --lib-name in manual kernel registration#12193Tanish2101 wants to merge 5 commits into
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.
Let's error out, if lib_name is specified but manual_registration is not enabled.
There was a problem hiding this comment.
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.
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.
Typo. Thank you for adding this docstring!
| if(GEN_LIB_NAME) | ||
| list(APPEND _gen_command --lib-name=${GEN_LIB_NAME}) | ||
| endif() |
There was a problem hiding this comment.
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.
Please use register_kernels_{lib name} format :)
There was a problem hiding this comment.
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.
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. |
larryliu0820
left a comment
There was a problem hiding this comment.
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 |
larryliu0820
left a comment
There was a problem hiding this comment.
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 |
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
|
Hi @Tanish2101, any update on this PR? |
|
@Tanish2101 pinging again for an update on the PR, thanks |
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
|
Hi @nil-is-all @Tanish2101 if the issue is still open I'd like to work on this. |
|
#20658 now addresses the work of this PR |

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