[microNPU] Add unary elementwise operator infrastructure with ABS #9530

ekalda · 2021-11-18T11:22:31Z

Added unary elementwise ABS legalization support and tests
Added unary_elementwise Relay to TIR lowering and tests
Added TIR to Vela translation and tests
Added codegen tests

Co-authored-by: Rishabh Jain [email protected]

ekalda · 2021-11-18T11:24:00Z

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

lhutton1

Great work @ekalda @jainris :) Just some minor comments

lhutton1 · 2021-11-20T13:30:31Z

python/tvm/relay/backend/contrib/ethosu/legalize.py

+        params = self.params_class(post.op.body)
+        params.ifm.tensor = post.args[0]
+
+        if str(params.ofm.layout) != "NHWC":


IIRC we agreed to remove these checks, please ignore if not

All the other operators still have these checks so maybe we can remove them in a follow-up patch for all of the operators (or to be precise, I think we need to add some guards into the is_valid() checks to prevent attempting offloading graphs with non-NHWC layout...)

No worries, I was referring to #9508 (comment) but didn't realize the other operators still had this check, I think a follow up would be fine :)

lhutton1 · 2021-11-20T13:33:17Z

python/tvm/relay/backend/contrib/ethosu/legalize.py

+            while len(unary_input_shape) < 4:
+                unary_input_shape = [1] + unary_input_shape


Suggested change

while len(unary_input_shape) < 4:

unary_input_shape = [1] + unary_input_shape

pad_size = 4 - len(unary_input_shape)

unary_input_shape = ([1] * pad_size) + unary_input_shape

lhutton1 · 2021-11-21T19:43:16Z

python/tvm/relay/backend/contrib/ethosu/tir_to_cs_translator.py

+) -> vapi.NpuElementWiseOperation:
+    """This function will translate a tir extern_call
+    as produced by Relay to TIR compilation.
+    Parameters


Nit: insert blank line

lhutton1 · 2021-11-21T19:46:51Z

python/tvm/relay/backend/contrib/ethosu/util.py

+    quantize arguments
+    """
+
+    ifm = 0


Looks like we missed this for binary elementwise also... should we follow convention here and use capitals?

Done (i.e. changed both to use capitals)

lhutton1 · 2021-11-21T19:53:21Z

src/relay/op/contrib/ethosu/unary_elementwise.cc

+  int ifm_index = 0;
+  int result_index = 2;


Better to make these const

lhutton1 · 2021-11-21T19:58:15Z

src/relay/op/contrib/ethosu/unary_elementwise.cc

+                              << operator_type;
+
+  auto ifm_dtype = ifm->dtype;
+  ICHECK(ifm_dtype == DataType::UInt(8) || ifm_dtype == DataType::Int(8))


Should we use the TypeReporter here instead, similar to other operators?

Well spotted :)

lhutton1 · 2021-11-21T19:59:19Z

src/relay/op/contrib/ethosu/unary_elementwise.cc

+  attrs->ifm_zero_point = ifm_zero_point;
+  attrs->ofm_scale = ofm_scale;
+  attrs->ofm_zero_point = ofm_zero_point;
+  attrs->ofm_channels = ofm_channels;


Suggested change

attrs->ofm_channels = ofm_channels;

attrs->ofm_channels = std::move(ofm_channels);

lhutton1 · 2021-11-21T19:59:37Z

src/relay/op/contrib/ethosu/unary_elementwise.cc

+                                int clip_max, String ifm_layout, String ofm_layout) {
+  auto attrs = make_object<EthosuUnaryElementwiseAttrs>();
+
+  attrs->operator_type = operator_type;


Suggested change

attrs->operator_type = operator_type;

attrs->operator_type = std::move(operator_type);

lhutton1 · 2021-11-21T20:09:00Z

tests/python/contrib/test_ethosu/test_legalize.py

+    mod, _ = relay.frontend.from_tflite(
+        tflite_model,
+        shape_dict={"input": ifm_shape},
+        dtype_dict={"x": dtype},


Suggested change

dtype_dict={"x": dtype},

dtype_dict={"input": dtype},

lhutton1 · 2021-11-21T20:12:05Z

tests/python/contrib/test_ethosu/test_type_inference.py

+    f = run_opt_pass(f, relay.transform.InferType())
+    assert tuple(f.body.checked_type.shape) == ofm_shape
+
+


We should also test invalid dtypes/shapes etc here

Should ABS have a rounding mode as implemented in #9514?

Yes, well spotted, I added the rounding_mode attribute :)

NicolaLancellotti · 2021-11-22T10:32:18Z

python/tvm/relay/backend/contrib/ethosu/legalize.py

+    ethosu_unary_elementwise operators
+    """
+
+    def __init__(self, params_class, pattern):


Suggested change

def __init__(self, params_class, pattern):

def __init__(self, params_class: Type, pattern: CallPattern):

NicolaLancellotti · 2021-11-22T10:45:51Z

python/tvm/relay/backend/contrib/ethosu/op/unary_elementwise.py

+    activation: str = "NONE",
+    clip_min: int = 0,
+    clip_max: int = 0,
+    ifm_layout: str = "NHWC",
+    ofm_layout: str = "NHWC",


Suggested change

activation: str = "NONE",

clip_min: int = 0,

clip_max: int = 0,

ifm_layout: str = "NHWC",

ofm_layout: str = "NHWC",

activation: Optional[str] = "NONE",

clip_min: Optional[int] = 0,

clip_max: Optional[int] = 0,

ifm_layout: Optional[str] = "NHWC",

ofm_layout: Optional[str] = "NHWC",

The docstring says that they are optional.
Actually, we can get the same behaviour with the default values and the None value. Maybe in a future pr we should have only one way to archive that (for the other operators too).

I added the type hints for the unary elementwise operator, maybe in the follow-up patch we can unify the use for all the operators?

NicolaLancellotti · 2021-11-22T11:02:19Z

python/tvm/relay/op/contrib/ethosu.py

+    """
+    A class to extract and store parameters of Abs in an NPU friendly way
+    """


The docstring should follow the same convention adopted with the other *Parms classes.

NicolaLancellotti · 2021-11-22T11:08:38Z

src/relay/op/contrib/ethosu/unary_elementwise.cc

+      << ifm_dtype;
+
+  // Assign ofm type
+  auto ofm_shape = EthosuInferBinaryElementwiseOutputShape(ifm->shape, param->ifm_layout,


We should rename the function EthosuInferBinaryElementwiseOutputShape if it is used for the unary operators too.

NicolaLancellotti · 2021-11-22T11:15:53Z

tests/python/contrib/test_ethosu/test_replace_unary_elementwise.py

+import tvm
+import tvm.script
+from tvm import relay
+from tvm.relay.testing import run_opt_pass
+from tvm.relay.backend.contrib.ethosu.tir import spec
+from tvm.relay.backend.contrib.ethosu.tir.compiler import lower_to_tir
+from .infra import make_ethosu_unary_elementwise
+
+import pytest


Suggested change

import tvm

import tvm.script

from tvm import relay

from tvm.relay.testing import run_opt_pass

from tvm.relay.backend.contrib.ethosu.tir import spec

from tvm.relay.backend.contrib.ethosu.tir.compiler import lower_to_tir

from .infra import make_ethosu_unary_elementwise

import pytest

import pytest

pytest.importorskip("ethosu.vela")

import tvm

import tvm.script

from tvm import relay

from tvm.relay.testing import run_opt_pass

from tvm.relay.backend.contrib.ethosu.tir import spec

from tvm.relay.backend.contrib.ethosu.tir.compiler import lower_to_tir

from .infra import make_ethosu_unary_elementwise

NicolaLancellotti · 2021-11-22T11:33:09Z

Should ABS have a rounding mode as implemented in #9514?

* Added unary elementwise ABS legalization support and tests * Added unary_elementwise Relay to TIR lowering and tests * Added TIR to Vela translation and tests * Added codegen tests Co-authored-by: Rishabh Jain <[email protected]>

Change-Id: Ic963f1237bdbb493006fbb7e02dc1e5949c7ba46

NicolaLancellotti

LGTM! Thanks, @ekalda.

lhutton1

LGTM!

manupak

LGTM!

manupak · 2021-11-22T22:10:27Z

Thanks all!

…ache#9530) * [microNPU] Add unary elementwise operator infrastructure with ABS * Added unary elementwise ABS legalization support and tests * Added unary_elementwise Relay to TIR lowering and tests * Added TIR to Vela translation and tests * Added codegen tests Co-authored-by: Rishabh Jain <[email protected]>

ekalda requested review from anijain2305, areusch, comaniac, icemelon, jroesch, junrushao, jwfromm, MarisaKirisame, mbrookhart, merrymercy, slyubomirsky, tqchen, vinx13, wweic, yzhliu, zhiics and ZihengJiang as code owners November 18, 2021 11:22

ekalda mentioned this pull request Nov 18, 2021

[Tracking Issue] Arm(R) Ethos(TM)-U NPU operator support #9410

Closed

15 tasks

lhutton1 reviewed Nov 22, 2021

View reviewed changes

NicolaLancellotti requested changes Nov 22, 2021

View reviewed changes

Rishabh Jain and others added 2 commits November 22, 2021 13:21

Responding to the review comments

415826e

Change-Id: Ic963f1237bdbb493006fbb7e02dc1e5949c7ba46

ekalda force-pushed the unary_elementwise_upstream branch from bfdf1f4 to 415826e Compare November 22, 2021 16:10

NicolaLancellotti approved these changes Nov 22, 2021

View reviewed changes

lhutton1 approved these changes Nov 22, 2021

View reviewed changes

manupak approved these changes Nov 22, 2021

View reviewed changes

manupak merged commit d061d7f into apache:main Nov 22, 2021

ekalda deleted the unary_elementwise_upstream branch November 23, 2021 09:25

driazati mentioned this pull request Jul 14, 2022

TVM v0.9.0.rc0 Release Candidate Notes #12102

Closed

		while len(unary_input_shape) < 4:
		unary_input_shape = [1] + unary_input_shape

	attrs->ofm_channels = ofm_channels;
	attrs->ofm_channels = std::move(ofm_channels);

	attrs->operator_type = operator_type;
	attrs->operator_type = std::move(operator_type);

		f = run_opt_pass(f, relay.transform.InferType())
		assert tuple(f.body.checked_type.shape) == ofm_shape

	def __init__(self, params_class, pattern):
	def __init__(self, params_class: Type, pattern: CallPattern):

[microNPU] Add unary elementwise operator infrastructure with ABS #9530

[microNPU] Add unary elementwise operator infrastructure with ABS #9530

Conversation

ekalda commented Nov 18, 2021

ekalda commented Nov 18, 2021

lhutton1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolaLancellotti Nov 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolaLancellotti commented Nov 22, 2021

NicolaLancellotti left a comment

Choose a reason for hiding this comment

lhutton1 left a comment

Choose a reason for hiding this comment

manupak left a comment

Choose a reason for hiding this comment

manupak commented Nov 22, 2021

NicolaLancellotti Nov 22, 2021 •

edited

Loading