-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Arm(R) Ethos(TM)-U NPU BinaryElementwise operators support #9442
Arm(R) Ethos(TM)-U NPU BinaryElementwise operators support #9442
Conversation
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.
Mostly looks good to me :)
python/tvm/relay/backend/contrib/ethosu/op/binary_elementwise.py
Outdated
Show resolved
Hide resolved
python/tvm/relay/backend/contrib/ethosu/te/binary_elementwise.py
Outdated
Show resolved
Hide resolved
input_data, | ||
output_data, | ||
accel_type, | ||
output_tolerance=1 if operator_type == "MAX" else 0, |
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 tolerance 1 for MAX?
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.
The Max operator with the RELU activation function is not bit accurate, it needs a tolerance of 1.
I'll investigate and address this problem in a follow-up pr.
def create_graph(): | ||
input1 = relay.var("x1", shape=ifm_shape, dtype=dtype) | ||
input2 = relay.var("x2", shape=ifm2_shape, dtype=dtype) | ||
c1 = relay.left_shift(input1, input2) |
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.
Since left shift legalization from Relay is tested, does it mean that the NPU left shift does the "actual" left shift, but right shift doesn't, so we shouldn't legalize the Relay right shift to Ethos-U right shift but should legalize the left shift? It seems a bit odd to me that one of the shifts has Relay legalization and the other doesn't, especially since I don't know what would be the use of Relay left shift for the NPU in the absence of corresponding TFLite operator. Any thoughts @manupa-arm @mbaret @lhutton1
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.
Can we not legalize a right shift operator to right shift followed by element-wise subtract of 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.
@ekalda Currently, all the NPU primitive operators are covered.
Because the NPU left shift operator does the "actual" left shift, we legalize the Relay left shift operator.
Conversely, the NPU right shift operator is actually a rounding right shift operator and so the Relay right shift operator cannot be legalized.
@lhutton1 The Relay right shift operator cannot be legalized to a rounding right shift followed by an element-wise subtract of 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.
I think we could follow-up on this in another PR
Change-Id: I18ff4ca7fc66dc283c798164b7f3e0af561f857d
efc47c1
to
bda3be0
Compare
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.
A heroic patch indeed :) I notice we have quite a lot of highly repetitious testing for very similar cases (a lot of times only the name of the operation is changing). Perhaps we could look and see if any such tests could be consolidated which might make maintenance of these tests a bit more straightforward.
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.
Thanks @NicolaLancellotti LGTM overall! Just some small things
python/tvm/relay/backend/contrib/ethosu/op/binary_elementwise.py
Outdated
Show resolved
Hide resolved
python/tvm/relay/backend/contrib/ethosu/te/binary_elementwise.py
Outdated
Show resolved
Hide resolved
python/tvm/relay/backend/contrib/ethosu/te/binary_elementwise.py
Outdated
Show resolved
Hide resolved
python/tvm/relay/backend/contrib/ethosu/te/binary_elementwise.py
Outdated
Show resolved
Hide resolved
Change-Id: I3a85a00702b890f4d09d64702614b048ceb2c8a9
Change-Id: I807c769c4fd3db412ae4e3397e8efbb6566a8838
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.
Thanks @NicolaLancellotti, LGTM!
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.
LGTM! :)
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.
LGTM
Thanks @NicolaLancellotti, @lhutton1, @ekalda, this is now merged. |
* main: (119 commits) [Topi][Op][PyTorch][Vitas] Fix inconsistent kernel layout conventions for conv2d_transpose (apache#9336) Fix repository URL in ubuntu_install_rocm.sh (apache#9425) Add LLVM-13 installation to Docker setup (apache#9498) [Relay] Use target_host determined at Relay level instead of recalculating it (apache#9499) Arm(R) Ethos(TM)-U NPU BinaryElementwise operators support (apache#9442) [COMMUNITY] Junru's and Wuwei's PGP key for ASF release (apache#9488) Add default for split op (apache#9489) [HOTFIX][TARGET] Change LOG in compilation config to DLOG (apache#9486) Fixed some warnings about lambda's closures that are bigger than necessary (apache#9481) [Support] Add libinfo into the runtime build (apache#9310) Change Call with TIRCallAttrs to call_lowered op (apache#9312) [ETHOSN] Streamline Ethos(TM)-N cross-compile rpc usage (apache#9477) [CMSIS-NN] Assert correct amount of CMSIS-NN artifacts in MLF (apache#9480) [MicroTVM][PyTest] Explicitly skip MicroTVM unittests. (apache#9335) [microNPU] Replace ICHECK with diagnostic context in type inference (apache#9470) Better host handling in CompilationConfig & debug printing (apache#9460) [AOT][Tests] Use pre-built libraries in Reference System tests (apache#9271) [TIR] Add type hint for TIR (apache#9432) [TVMC] Add test for quantized pytorch model (apache#9467) [CMSIS-NN] Convert CMSIS-NN to use Target Hooks (apache#9397) ...
This commit adds support for the binary elementwise primitive operators for the Arm(R) Ethos(TM)-U NPU and includes a few minor rewording changes.
This commit adds support for the binary elementwise primitive operators for the Arm(R) Ethos(TM)-U NPU and includes a few minor rewording changes.
This commit adds support for the binary elementwise primitive operators for the Arm(R) Ethos(TM)-U NPU and includes a few minor rewording changes.
This commit adds support for the binary elementwise primitive operators for the Arm(R) Ethos(TM)-U NPU and includes a few minor rewording changes.
This commit adds support for the binary elementwise primitive operators for the Arm(R) Ethos(TM)-U NPU and includes a few minor rewording changes.
This commit adds support for the binary elementwise primitive operators for the Arm(R) Ethos(TM)-U NPU and includes a few minor rewording changes.