Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[microNPU] Add support for unary elementwise CLZ #9577

Merged
merged 2 commits into from
Nov 26, 2021
Merged

[microNPU] Add support for unary elementwise CLZ #9577

merged 2 commits into from
Nov 26, 2021

Conversation

ekalda
Copy link
Contributor

@ekalda ekalda commented Nov 24, 2021

Add support for the CLZ (count leading zeros) operator
and the codegen test.

Co-authored-by: Rishabh Jain rishabh.jain2@arm.com

@ekalda
Copy link
Contributor Author

ekalda commented Nov 24, 2021

@manupa-arm @mbaret @lhutton1 @NicolaLancellotti @dchauhan-arm

NicolaLancellotti
NicolaLancellotti reviewed Nov 24, 2021
View reviewed changes
python/tvm/relay/backend/contrib/ethosu/te/unary_elementwise.py
Comment on lines +115 to +117
def clz_imp(inp):
# Assuming that it's a 32 bit int
return 32 - te.log2(inp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can inp be negative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the inp can be negative, but for the operators we turn into extern_calls the actual correctness of the compute expression doesn't really matter since we will pass to the NPU the parameters of the operation, but no specifics about the actual compute. That's probably why a "close enough" operation is used here...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right.

manupak
manupak approved these changes Nov 25, 2021
View reviewed changes
Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

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

LGTM modulo the existing comments.
I think to avoid the rebasing overhead -- we could address the comments in a follow up.

@manupak
Copy link
Contributor

manupak commented Nov 25, 2021

oh bummer, it already have a conflict.

@ekalda ekalda mentioned this pull request Nov 25, 2021
Closed
15 tasks
@ekalda
Copy link
Contributor Author

ekalda commented Nov 25, 2021

oh bummer, it already have a conflict.

It's a very simple rebasing fix, but considering the CI is broken now, there probably isn't much point in updating the PR before it is up again...

NicolaLancellotti
NicolaLancellotti approved these changes Nov 25, 2021
View reviewed changes
Copy link
Contributor

@NicolaLancellotti NicolaLancellotti left a comment

Choose a reason for hiding this comment

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

LGTM

lhutton1
lhutton1 approved these changes Nov 25, 2021
View reviewed changes
Copy link
Contributor

@lhutton1 lhutton1 left a comment

Choose a reason for hiding this comment

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

Just curious why tolerance of 1 is used? Otherwise LGTM!

Rishabh Jain and others added 2 commits November 26, 2021 09:13
@ekalda
[microNPU] Add support for unary elementwise CLZ
79cde3e 
Add support for the CLZ (count leading zeros) operator
and the codegen test.

Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com>
@ekalda
Removing output tolerance 1
8fe5cbe 
Change-Id: I4f4189c2b5f864117b85dbd31fd67ae1680dc13a
@ekalda
Copy link
Contributor Author

ekalda commented Nov 26, 2021

Just curious why tolerance of 1 is used? Otherwise LGTM!

Ah well spotted, there is not reason to use tolerance one (I'd be quite worried if NPU couldn't count zeros accurately 😅 ). Removed it!

@manupak manupak merged commit a82abd4 into apache:main Nov 26, 2021
@manupak
Copy link
Contributor

manupak commented Nov 26, 2021

Thanks @ekalda @lhutton1 @NicolaLancellotti ! Great work!

@ekalda ekalda deleted the clz_upstream branch November 26, 2021 16:30
dchauhan-arm pushed a commit to dchauhan-arm/tvm that referenced this pull request Nov 29, 2021
@ekalda @dchauhan-arm
[microNPU] Add support for unary elementwise CLZ (apache#9577)
7597bdd 
Add support for the CLZ (count leading zeros) operator
and the codegen test.


Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com>
masahi pushed a commit to masahi/tvm that referenced this pull request Dec 1, 2021
@ekalda @masahi
[microNPU] Add support for unary elementwise CLZ (apache#9577)
1498239 
Add support for the CLZ (count leading zeros) operator
and the codegen test.


Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com>
mehrdadh pushed a commit to mehrdadh/tvm that referenced this pull request Dec 1, 2021
@ekalda @mehrdadh
[microNPU] Add support for unary elementwise CLZ (apache#9577)
9ff9cbd 
Add support for the CLZ (count leading zeros) operator
and the codegen test.


Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com>
mehrdadh pushed a commit to mehrdadh/tvm that referenced this pull request Dec 1, 2021
@ekalda @mehrdadh
[microNPU] Add support for unary elementwise CLZ (apache#9577)
8fea2f4 
Add support for the CLZ (count leading zeros) operator
and the codegen test.


Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com>
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
@ekalda @ylc
[microNPU] Add support for unary elementwise CLZ (apache#9577)
b01d513 
Add support for the CLZ (count leading zeros) operator
and the codegen test.


Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com>
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 11, 2022
@ekalda @yangulei
[microNPU] Add support for unary elementwise CLZ (apache#9577)
4b5d35b 
Add support for the CLZ (count leading zeros) operator
and the codegen test.


Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com>
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 12, 2022
@ekalda @yangulei
[microNPU] Add support for unary elementwise CLZ (apache#9577)
964a504 
Add support for the CLZ (count leading zeros) operator
and the codegen test.


Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com>
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
@ekalda @ylc
[microNPU] Add support for unary elementwise CLZ (apache#9577)
6499509 
Add support for the CLZ (count leading zeros) operator
and the codegen test.


Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NicolaLancellotti NicolaLancellotti NicolaLancellotti approved these changes

@lhutton1 lhutton1 lhutton1 approved these changes

@manupak manupak manupak approved these changes

@anijain2305 anijain2305 Awaiting requested review from anijain2305

@areusch areusch Awaiting requested review from areusch

@comaniac comaniac Awaiting requested review from comaniac

@icemelon icemelon Awaiting requested review from icemelon

@jroesch jroesch Awaiting requested review from jroesch

@junrushao junrushao Awaiting requested review from junrushao

@jwfromm jwfromm Awaiting requested review from jwfromm

@MarisaKirisame MarisaKirisame Awaiting requested review from MarisaKirisame

@mbrookhart mbrookhart Awaiting requested review from mbrookhart

@merrymercy merrymercy Awaiting requested review from merrymercy

@slyubomirsky slyubomirsky Awaiting requested review from slyubomirsky

@tqchen tqchen Awaiting requested review from tqchen

@vinx13 vinx13 Awaiting requested review from vinx13

@wweic wweic Awaiting requested review from wweic

@yzhliu yzhliu Awaiting requested review from yzhliu

@zhiics zhiics Awaiting requested review from zhiics

@ZihengJiang ZihengJiang Awaiting requested review from ZihengJiang

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ekalda @manupak @NicolaLancellotti @lhutton1