Skip to content

Introducing NXP Neutron runtime #10563

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jirioc
Copy link
Collaborator

@jirioc jirioc commented Apr 29, 2025

Summary

Introducing NXP Neutron runtime.

cc @digantdesai @JakeStevens @robert-kalmar

Copy link

pytorch-bot bot commented Apr 29, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10563

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures

As of commit 8a79f2d with merge base 91c9ffa (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 Apr 29, 2025
@jirioc
Copy link
Collaborator Author

jirioc commented Apr 30, 2025

@pytorchbot label "module: nxp" "release notes: nxp"

@pytorch-bot pytorch-bot bot added module: nxp Issues related to NXP Neutron NPU delegation and code under backends/nxp/ release notes: nxp Changes to the NXP Neutron backend delegate labels Apr 30, 2025
@jirioc
Copy link
Collaborator Author

jirioc commented Apr 30, 2025

@tarun292
Copy link
Contributor

tarun292 commented May 2, 2025

@digantdesai are you the POC for reviewing this on our end?

Copy link
Contributor

@digantdesai digantdesai left a comment

Choose a reason for hiding this comment

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

Is this still in progress?

At a high level looks OK to me.

/// - Initialize the Neutron Driver library, setting initial values, do memory
/// allocation
/// for internal data structures, do memory mapping.
NeutronError neutronInit();
Copy link
Contributor

Choose a reason for hiding this comment

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

can these functions be in a neutron namespace?
Where are we calling this specific function to init the lib?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is called in surrounding rtos code, much like ethosu_init


// Applied on outputs.
template <typename T>
void transposeToChannelFirst(
Copy link
Contributor

Choose a reason for hiding this comment

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

OK for now, but I wonder is there another way to do this, i.e. set the dim_order on the output and let the portable or someone else take care of this?

Copy link
Contributor

Choose a reason for hiding this comment

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

From what @jirioc explained below, I think this could be removed by inserting a transpose node instead of setting a flag


// Applied on inputs.
template <typename T>
void transposeToChannelLast(
Copy link
Contributor

Choose a reason for hiding this comment

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

same here.


auto* cfg = allocator->allocateInstance<NeutronConfig>();

// The following data is read from the "processed" data blob.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: make this a helper function?

Copy link
Contributor

@JakeStevens JakeStevens left a comment

Choose a reason for hiding this comment

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

LGTM, still outstanding seems to be:

  1. namespace for neutron backend
  2. consider removing NeutronDriver/NeutronErrors.h from ET repo and pull in to link separately like ARM does

/// - Initialize the Neutron Driver library, setting initial values, do memory
/// allocation
/// for internal data structures, do memory mapping.
NeutronError neutronInit();
Copy link
Contributor

Choose a reason for hiding this comment

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

It is called in surrounding rtos code, much like ethosu_init


// Applied on outputs.
template <typename T>
void transposeToChannelFirst(
Copy link
Contributor

Choose a reason for hiding this comment

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

From what @jirioc explained below, I think this could be removed by inserting a transpose node instead of setting a flag

@digantdesai
Copy link
Contributor

Any update on this?

@robert-kalmar
Copy link
Collaborator

robert-kalmar commented May 13, 2025

LGTM, still outstanding seems to be:

1. namespace for neutron backend

Thanks for advice, will do

2. consider removing NeutronDriver/NeutronErrors.h from ET repo and pull in to link separately like ARM does

So far we don't have the NeutronDriver/NeutronErrors.h. It is part of the ExecuTorch example in the MCUXpresso SDK but I can imagine to have it distributed alongside with the Neutron Converter, Neutron Functional Simulator or as a separated part of the SDK. Let us check internally and update as we discuss it internally.

Copy link
Contributor

@digantdesai digantdesai left a comment

Choose a reason for hiding this comment

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

I guess transpose is the only fix pending? I am ok with checking in header file for now.

@jirioc
Copy link
Collaborator Author

jirioc commented May 14, 2025

The namespace has been added.

@JakeStevens
Copy link
Contributor

JakeStevens commented May 14, 2025

I guess transpose is the only fix pending? I am ok with checking in header file for now.

FWIW, it is how ARM does it currently as well.

executorch/backends/arm/runtime/EthosUBackend.cpp

If there are well supported tests for the runtime, this might be a good candidate for a task for new contributors interested in ET.

@robert-kalmar
Copy link
Collaborator

robert-kalmar commented May 15, 2025

I guess transpose is the only fix pending? I am ok with checking in header file for now.

FWIW, it is how ARM does it currently as well.

executorch/backends/arm/runtime/EthosUBackend.cpp

If there are well supported tests for the runtime, this might be a good candidate for a task for new contributors interested in ET.

Compared to Arm's approach we carry the transposition information with us in the Neutron Payload's header, as we get this by running the "tensor format inference" instead of Dim Orders.

@digantdesai
Copy link
Contributor

Not sure I fully understand the need for transposing input/output tensors in the delegate runtime unless you want to be flexible to in terms of supporting both dim_orders, but ET doesn't support that i.e. it specializes on the dim_order at PTE generation time. So you can also use the portable op for transposing through a pass instead of this. But this might also work.

Also can we add at least a build test with armv8-m toolchain?

@digantdesai
Copy link
Contributor

Let's re-merge the CI PR, this PR depends on that (touches trunk.yaml), then we can merge this. Thanks.

@jirioc jirioc force-pushed the EIEX-289-upstream-neutron-runtime branch from 9534c72 to 8a79f2d Compare June 24, 2025 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. module: nxp Issues related to NXP Neutron NPU delegation and code under backends/nxp/ release notes: nxp Changes to the NXP Neutron backend delegate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants