Skip to content

Added support for --lib-name in manual kernel registration#12193

Closed
Tanish2101 wants to merge 5 commits into
pytorch:mainfrom
Tanish2101:manual-registration-lib-name-11221
Closed

Added support for --lib-name in manual kernel registration#12193
Tanish2101 wants to merge 5 commits into
pytorch:mainfrom
Tanish2101:manual-registration-lib-name-11221

Conversation

@Tanish2101

@Tanish2101 Tanish2101 commented Jul 3, 2025

Copy link
Copy Markdown

Summary

This PR adds support for explicit function-based kernel registration in ExecuTorch through a new --lib-name flag in the code generation workflow. This allows each kernel library to generate a register_<lib_name>_kernels() function instead of relying on static constructors and -force_load, which improves build portability especially for platforms like Xcode where -force_load needs to be used currently.

Key changes:

  1. Introduced --lib-name option for --manual-registration mode in codegen.

  2. Generates register_<lib_name>_kernels() in RegisterKernels.cpp and corresponding headers.

  3. Extended documentation under kernel-library-selective-build.md to describe the new registration mechanism.

Fixes #11221

@larryliu0820 @shoumikhin

@pytorch-bot

pytorch-bot Bot commented Jul 3, 2025

Copy link
Copy Markdown

🔗 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 Failures

As of commit d83a541 with merge base ef48cc2 (image):

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 3, 2025
@github-actions

github-actions Bot commented Jul 3, 2025

Copy link
Copy Markdown

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@Tanish2101

Tanish2101 commented Jul 3, 2025

Copy link
Copy Markdown
Author

@pytorchbot label "release notes: none"

@pytorch-bot

pytorch-bot Bot commented Jul 3, 2025

Copy link
Copy Markdown

Didn't find following labels among repository labels: Release Notes: ops & kernels

@shoumikhin

Copy link
Copy Markdown
Contributor

Thanks for the change!

Wdyt if we call the newly exposed symbol as register_kerlens_ to match the Apple frameworks product naming?

@larryliu0820

Copy link
Copy Markdown
Contributor

Thank you for submitting PR! Let me take a look

Comment thread codegen/gen.py
kernel_index=kernel_index,
manual_registration=options.manual_registration,
add_exception_boundary=options.add_exception_boundary,
lib_name=options.lib_name,

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread codegen/templates/RegisterKernels.cpp Outdated
::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");

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

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!

Comment thread tools/cmake/Codegen.cmake Outdated
Comment on lines +145 to +147
if(GEN_LIB_NAME)
list(APPEND _gen_command --lib-name=${GEN_LIB_NAME})
endif()

Copy link
Copy Markdown
Contributor

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:

  1. Using static initializer to register all kernels (default option)
  2. 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?

@shoumikhin shoumikhin Jul 3, 2025

Copy link
Copy Markdown
Contributor

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 :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it matter?

@shoumikhin shoumikhin Jul 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that it matches the packages naming, see my comment to the original proposal issue.
Plus we're gonna have manual registration for backends too seems like, so having it like register_{kernels/backend}_{name} sounds right

image

Copy link
Copy Markdown
Author

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:

  1. Using static initializer to register all kernels (default option)
  2. 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?

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?

Copy link
Copy Markdown
Contributor

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.

@lucylq

lucylq commented Jul 3, 2025

Copy link
Copy Markdown
Contributor

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

@Tanish2101

Tanish2101 commented Jul 9, 2025

Copy link
Copy Markdown
Author

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

@larryliu0820 @shoumikhin @lucylq

@lucylq

lucylq commented Jul 10, 2025

Copy link
Copy Markdown
Contributor

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

@larryliu0820 @shoumikhin @lucylq

Hi @Tanish2101, can you try rebasing on top of main, and picking only your changes?
i.e., you can try git rebase -i main, and only attach your commits.

@Tanish2101
Tanish2101 force-pushed the manual-registration-lib-name-11221 branch from b47f474 to 4e0bac5 Compare July 11, 2025 13:34
@Tanish2101

Copy link
Copy Markdown
Author

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
@larryliu0820 @shoumikhin @lucylq

Hi @Tanish2101, can you try rebasing on top of main, and picking only your changes? i.e., you can try git rebase -i main, and only attach your commits.

Thanks @lucylq now the problem is resolved. 👍

@Tanish2101

Copy link
Copy Markdown
Author

Please check my changes now.
@larryliu0820 @shoumikhin

larryliu0820
larryliu0820 previously approved these changes Jul 11, 2025

@larryliu0820 larryliu0820 left a comment

Copy link
Copy Markdown
Contributor

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

@larryliu0820

Copy link
Copy Markdown
Contributor

Can you resolve the conflict and I can help kicking off the CI

@Tanish2101 Tanish2101 closed this Jul 12, 2025
@Tanish2101 Tanish2101 reopened this Jul 12, 2025
@pytorch-bot
pytorch-bot Bot dismissed larryliu0820’s stale review July 12, 2025 07:27

This PR was reopened (likely due to being reverted), so your approval was removed. Please request another review.

@Tanish2101

Copy link
Copy Markdown
Author

Can you resolve the conflict and I can help kicking off the CI

I have already resolved those conflicts which occured in kernel-library-selective-build.md file, I don't know why it is still showing the conflict.

@larryliu0820 larryliu0820 left a comment

Copy link
Copy Markdown
Contributor

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

@Tanish2101

Copy link
Copy Markdown
Author

I can take a look on Monday

Please let me know if there are any changes required.

@larryliu0820

Copy link
Copy Markdown
Contributor

ok resolved the conflict. Waiting for CI signals before I merge this

@larryliu0820

Copy link
Copy Markdown
Contributor

@Tanish2101 seeing this error:

Traceback (most recent call last):
  File "/opt/conda/envs/py_3.10/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/opt/conda/envs/py_3.10/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/pytorch/executorch/codegen/gen.py", line 1053, in <module>
    main()
  File "/pytorch/executorch/codegen/gen.py", line 1008, in main
    gen_headers(
  File "/pytorch/executorch/codegen/gen.py", line 530, in gen_headers
    cpu_fm.write(
  File "/opt/conda/envs/py_3.10/lib/python3.10/site-packages/torchgen/utils.py", line 229, in write
    self.write_with_template(filename, filename, env_callable)
  File "/opt/conda/envs/py_3.10/lib/python3.10/site-packages/torchgen/utils.py", line 218, in write_with_template
    substitute_out = self.substitute_with_template(
  File "/opt/conda/envs/py_3.10/lib/python3.10/site-packages/torchgen/utils.py", line 184, in substitute_with_template
    substitute_out = template.substitute(env)
  File "/opt/conda/envs/py_3.10/lib/python3.10/site-packages/torchgen/code_template.py", line 83, in substitute
    return self.substitution.sub(replace, self.pattern)
  File "/opt/conda/envs/py_3.10/lib/python3.10/site-packages/torchgen/code_template.py", line 70, in replace
    v = lookup(key)
  File "/opt/conda/envs/py_3.10/lib/python3.10/site-packages/torchgen/code_template.py", line 47, in lookup
    return kwargs[v] if v in kwargs else env[v]
KeyError: 'lib_name'

Can you take a look?

To repro this, run:

cmake --preset linux
cmake --build cmake-out -j7

@Tanish2101

Copy link
Copy Markdown
Author

@Tanish2101 seeing this error:

Traceback (most recent call last):
  File "/opt/conda/envs/py_3.10/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/opt/conda/envs/py_3.10/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/pytorch/executorch/codegen/gen.py", line 1053, in <module>
    main()
  File "/pytorch/executorch/codegen/gen.py", line 1008, in main
    gen_headers(
  File "/pytorch/executorch/codegen/gen.py", line 530, in gen_headers
    cpu_fm.write(
  File "/opt/conda/envs/py_3.10/lib/python3.10/site-packages/torchgen/utils.py", line 229, in write
    self.write_with_template(filename, filename, env_callable)
  File "/opt/conda/envs/py_3.10/lib/python3.10/site-packages/torchgen/utils.py", line 218, in write_with_template
    substitute_out = self.substitute_with_template(
  File "/opt/conda/envs/py_3.10/lib/python3.10/site-packages/torchgen/utils.py", line 184, in substitute_with_template
    substitute_out = template.substitute(env)
  File "/opt/conda/envs/py_3.10/lib/python3.10/site-packages/torchgen/code_template.py", line 83, in substitute
    return self.substitution.sub(replace, self.pattern)
  File "/opt/conda/envs/py_3.10/lib/python3.10/site-packages/torchgen/code_template.py", line 70, in replace
    v = lookup(key)
  File "/opt/conda/envs/py_3.10/lib/python3.10/site-packages/torchgen/code_template.py", line 47, in lookup
    return kwargs[v] if v in kwargs else env[v]
KeyError: 'lib_name'

Can you take a look?

To repro this, run:

cmake --preset linux
cmake --build cmake-out -j7

Ok, I will check.

@github-actions

Copy link
Copy Markdown

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions Bot added the Stale PRs inactive for over 60 days label Sep 21, 2025
@github-actions

Copy link
Copy Markdown

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@nil-is-all

Copy link
Copy Markdown
Contributor

Hi @Tanish2101, any update on this PR?

@nil-is-all

Copy link
Copy Markdown
Contributor

@Tanish2101 pinging again for an update on the PR, thanks

@github-actions

Copy link
Copy Markdown

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions Bot closed this Apr 29, 2026
@XAheli

XAheli commented May 6, 2026

Copy link
Copy Markdown
Contributor

Hi @nil-is-all @Tanish2101 if the issue is still open I'd like to work on this.

@nil-is-all

Copy link
Copy Markdown
Contributor

#20658 now addresses the work of this PR

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

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Stale PRs inactive for over 60 days

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Manual kernel registration to include library names in API

7 participants