NXP backend: Enable MM and AddMM with new Neutron flow.#20278
NXP backend: Enable MM and AddMM with new Neutron flow.#20278MartinPavella wants to merge 6 commits into
MM and AddMM with new Neutron flow.#20278Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/20278
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 2 Unrelated FailuresAs of commit b10f3f9 with merge base 6021a58 ( NEW FAILURE - The following job has failed:
BROKEN TRUNK - The following jobs failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
|
@novak-vaclav, @irtrukhina please feel free to have a look as well. |
9c886de to
3495635
Compare
3495635 to
605f986
Compare
3044d1d to
896a8c2
Compare
roman-janik-nxp
left a comment
There was a problem hiding this comment.
LGTM, need to wait on internal tests.
896a8c2 to
e3aa36b
Compare
|
Internal tests: https://bamboo3.sw.nxp.com/browse/MLTECE-EXIGH188-8 |
novak-vaclav
left a comment
There was a problem hiding this comment.
I have just a few comments and questions, otherwise very good 👌
|
|
||
|
|
||
| # The edge operator signature is: aten.addmm(bias, input, weight, *, beta=1, alpha=1) | ||
| MAIN_INPUT_IDX = 1 |
| def convert(self, node: Node): | ||
| """Convert the `aten.addmm` operator to NeutronIR `FullyConnected`. | ||
| The schema is: | ||
| addmm( |
There was a problem hiding this comment.
nit: this is the schema of high level torch API, however in aten, the addmm operator's signature looks different.
I find this a bit confusing, since the node_converter deals with only aten operators.
There was a problem hiding this comment.
There was a problem hiding this comment.
What I had in mind was I'd rather see directly the "schema" of node.args, since that is what we directly work with in node_converter. Something like node.args = [input_node, mat1, ...].
I thought the schema was taken from the documentation online, which might be less reliable than directly identifing the nodes in node.args. However if it is taken from the node attribute directly, then it's fine by me.
There was a problem hiding this comment.
I'm just thinking, we could make a precommit hook that would check that for each modified file in a commit, if there is an NXP license, the year must be up to date.
Something like: get modified files -> for each file, read the first line -> if the line starts with "# Copyright", it must also contain "2026"
What do you think? I don't know if we can just create precommit hooks in an opensource repository. An alternative might be a special CI test in .github/workflows?
There was a problem hiding this comment.
That's a good idea!
Just a note, only "significant" changes to a file require the license to be updated. The word "significant" is vague and up to our interpretation (for example for the changes to test_integration.py here, I deemed the changes insignificant, as I just added a comment, updates some indices, and replaced operators with their aliases. No functional changes were made. But someone else may consider this significant anyway.).
Regarding the automatic check, please feel free to bring it up during the architecture meeting on Monday. I quite like it :)
e3aa36b to
b10f3f9
Compare

Summary
This PR enables the
MMandAddMMoperators in the NXP backend using the new Neutron C MLIR flow.Test plan
Unit tests provided.
cc @robert-kalmar @JakeStevens @digantdesai @rascani