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.

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

[microNPU] Add support for SIGMOID #9627

Merged
merged 3 commits into from
Dec 8, 2021
Merged

Conversation

ekalda
Copy link
Contributor

@ekalda ekalda commented Dec 1, 2021

Add support for SIGMOID activation function using the lookup
table mechanism in the NPU.

@ekalda
Copy link
Contributor Author

ekalda commented Dec 1, 2021

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.

LGTM Overall! Just a couple minor things I noticed :)

return y


class SigmoidRewriter(DFPatternCallback):
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks very similar to TanhRewriter, is it worth creating a subclass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I made the change


def is_valid(self):
"""
This function checks whether reshape has compatible attributes with the NPU
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This function checks whether reshape has compatible attributes with the NPU
This function checks whether sigmoid has compatible attributes with the NPU

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


class Model(tf.Module):
@tf.function
def tanh_function(self, x):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def tanh_function(self, x):
def sigmoid_function(self, x):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

dtype = "int8"

def create_tflite_graph():
tf.config.run_functions_eagerly(True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tf.config.run_functions_eagerly(True)

We don't need it, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we don't :D

@ekalda
Copy link
Contributor Author

ekalda commented Dec 6, 2021

Thanks for the review @lhutton1!

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.

Some type hints are missing, could you add them?

)(wildcard())
self.pattern = (wildcard().has_attr({"Composite": params_class.composite_name}))(wildcard())
self.activation_type = activation_type
self.calc_func = calc_func

def callback(self, pre, post, node_map):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def callback(self, pre, post, node_map):
def callback(self, pre: tvm.relay.Expr, post: tvm.relay.Expr, node_map: tvm.ir.container.Map):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -194,6 +205,48 @@ def __call__(self, *args, **kwargs):
pass


def sigmoid_calc_func(x):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def sigmoid_calc_func(x):
def sigmoid_calc_func(x: float) -> float:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


def __init__(self):
def __init__(self, params_class, activation_type, calc_func):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def __init__(self, params_class, activation_type, calc_func):
def __init__(self, params_class: Type, activation_type: string, calc_func: Callable[[float], float]):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -125,30 +125,30 @@ def __call__(self, *args, **kwargs):
pass


def find_tanh_values(ifm_scale, ifm_zp, ofm_scale, ofm_zp):
"""Method to calculate the values of the tanh lookup table"""
def get_lut_from_func(ifm_scale, ifm_zp, ofm_scale, ofm_zp, func):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def get_lut_from_func(ifm_scale, ifm_zp, ofm_scale, ofm_zp, func):
def get_lut_from_func(ifm_scale: float, ifm_zp: int, ofm_scale: float, ofm_zp: int, func: Callable[[float], float]) -> list[int]:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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.

Apologies, something I missed in my first review. Not essential, but given @NicolaLancellotti's review I suppose there's no harm in including this also :)

@@ -1035,6 +1035,35 @@ def mean_pattern() -> tvm.relay.dataflow_pattern.DFPattern:
return pattern


class SigmoidParams:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also create a subclass for this and TanhParams?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Add support for SIGMOID activation function using the lookup
table mechanism in the NPU.
@ekalda
Copy link
Contributor Author

ekalda commented Dec 7, 2021

Thanks for the reviews @NicolaLancellotti and @lhutton1! :)

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.

Thanks @ekalda, LGTM!

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!

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.

@manupak manupak merged commit 3371a76 into apache:main Dec 8, 2021
@manupak
Copy link
Contributor

manupak commented Dec 8, 2021

Thanks all!

@ekalda ekalda deleted the sigmoid_upstream branch December 8, 2021 11:27
mikepapadim pushed a commit to mikepapadim/tvm that referenced this pull request Dec 9, 2021
Add support for SIGMOID activation function using the lookup
table mechanism in the NPU.
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
Add support for SIGMOID activation function using the lookup
table mechanism in the NPU.
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 11, 2022
Add support for SIGMOID activation function using the lookup
table mechanism in the NPU.
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 12, 2022
Add support for SIGMOID activation function using the lookup
table mechanism in the NPU.
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
Add support for SIGMOID activation function using the lookup
table mechanism in the NPU.
qsqqsqqsq-intellif pushed a commit to qsqqsqqsq-intellif/tvm that referenced this pull request Apr 29, 2022
Add support for SIGMOID activation function using the lookup
table mechanism in the NPU.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants