ashutosh-arm commented on code in PR #12887:
URL: https://github.com/apache/tvm/pull/12887#discussion_r980029874
##########
python/tvm/relay/op/contrib/ethosn.py:
##########
@@ -227,11 +227,13 @@ def qnn_add_pattern():
is_constant(),
is_constant(),
)
- two_inputs = gen_add_inputs(wildcard(), wildcard())
- input_is_left = gen_add_inputs(wildcard(), is_constant())
- input_is_right = gen_add_inputs(is_constant(), wildcard())
- return input_is_left | input_is_right | two_inputs
+ if has_constant_input:
+ input_is_left = gen_add_inputs(wildcard(), is_constant())
+ input_is_right = gen_add_inputs(is_constant(), wildcard())
+ return input_is_left | input_is_right
+ else:
+ return gen_add_inputs(wildcard(), wildcard())
Review Comment:
Since all types of inputs are supported, can the function simply return with
an op check and ignore the input types?
##########
tests/python/contrib/test_ethosn/test_convert_equivalents.py:
##########
@@ -74,7 +74,7 @@ def before():
relay.const(output_sc, "float32"),
relay.const(output_zp, "int32"),
)
- composite = tei.make_ethosn_composite(expr, "ethos-n.qnn_mul")
Review Comment:
Same as one of the previously asked question: are the direct TVM mul -> NPU
MUL test cases not needed any more?
##########
python/tvm/relay/op/contrib/ethosn.py:
##########
@@ -299,16 +301,24 @@ def check_leaky_relu(extract):
return _ethosn.leaky_relu(extract)
- def check_mul(extract):
- """Check if Mul is supported."""
+ def check_mul_to_reinterpret_quantize(extract):
+ """Check if Mul is supported by converting to reinterpret quantize"""
if not ethosn_available():
return False
- # Do not support scalar constants for now
- check_scalar = lambda i: isinstance(i, tvm.relay.Constant) and
len(i.data.shape) == 0
- if check_scalar(extract.args[0]) or check_scalar(extract.args[1]):
Review Comment:
What is the behavior of TVM when both the inputs are constants. Does it not
reach here? If it does, then does it need more simplifications to avoid getting
offloaded to support library?
##########
src/relay/backend/contrib/ethosn/convert_equivalent.cc:
##########
@@ -39,100 +39,117 @@ namespace relay {
namespace contrib {
namespace ethosn {
+/*!
+ * \brief Helper class to extract inputs and quantization information from
binary
+ * elementwise operations ready to convert.
+ */
+class BinaryElementwiseParams {
+ public:
+ static BinaryElementwiseParams ExtractBinaryElementwiseParams(const Call&
call) {
+ auto params = BinaryElementwiseParams();
+ params.input1 = call->args[0];
+ params.input2 = call->args[1];
+ params.input1_scale = call->args[2];
+ params.input1_zero_point = call->args[3];
+ params.input2_scale = call->args[4];
+ params.input2_zero_point = call->args[5];
+ // Reverse the inputs if the constant is first input
+ if (call->args[0]->IsInstance<ConstantNode>()) {
+ params.input1 = call->args[1];
+ params.input2 = call->args[0];
+ params.input1_scale = call->args[4];
+ params.input1_zero_point = call->args[5];
+ params.input2_scale = call->args[2];
+ params.input2_zero_point = call->args[3];
+ }
+ params.output_scale = call->args[6];
+ params.output_zero_point = call->args[7];
+ return params;
+ }
+
+ Expr input1;
+ Expr input2;
+ Expr input1_scale;
+ Expr input1_zero_point;
+ Expr input2_scale;
+ Expr input2_zero_point;
+ Expr output_scale;
+ Expr output_zero_point;
Review Comment:
Would it be better to have the members as constants to avoid duplication of
casts where this struct is consumed? I can imagine its not possible to do the
same for inputs.
##########
tests/python/contrib/test_ethosn/test_addition.py:
##########
@@ -117,13 +119,15 @@ def test_addition(dtype, shape):
@requires_ethosn
@pytest.mark.parametrize("dtype", ["uint8", "int8"])
@pytest.mark.parametrize(
- "lhs_shape,rhs_shape",
+ "lhs_shape,lhs_is_constant,rhs_shape,rhs_is_constant",
[
- ((1, 4, 4, 8), (1, 1, 1, 8)),
- ((1, 16, 12, 4), (4,)),
+ ((1, 4, 4, 8), False, (1, 1, 1, 8), True),
+ ((4,), True, (1, 16, 12, 4), False),
+ ((1, 1, 1, 8), True, (1, 4, 4, 8), False),
+ ((1, 16, 12, 4), False, (4,), True),
Review Comment:
do we need to test for true, true case as well?
##########
python/tvm/relay/op/contrib/ethosn.py:
##########
@@ -328,19 +338,40 @@ def check_add(extract):
"""Check if an addition is supported by Ethos-N."""
if not ethosn_available():
return False
- # Do not support scalar constants for now
- check_scalar = lambda i: isinstance(i, tvm.relay.Constant) and
len(i.data.shape) == 0
- if check_scalar(extract.args[0]) or check_scalar(extract.args[1]):
- return False
- inputs = extract.args[0:2]
- if any([isinstance(i, tvm.relay.Constant) for i in inputs]):
- extract = _ethosn.ConvertQnnAdd(extract)
- return _ethosn.conv2d(extract)
return _ethosn.addition(extract)
+ def check_add_to_reinterpret_quantize(extract):
+ """Check if addition can be converted to a reinterpret quantize
operation."""
+ if not ethosn_available():
+ return False
+ converted_extract = _ethosn.ConvertQnnAddToReinterpretQuantize(extract)
+ if converted_extract:
+ return _ethosn.reinterpret_quantize(converted_extract)
+ return False
+
+ def check_add_to_depthwise(extract):
+ """Check if addition can be converted to a depthwise operation."""
+ if not ethosn_available():
+ return False
+ converted_extract = _ethosn.ConvertQnnAddToDepthwise(extract)
+ if converted_extract:
+ return _ethosn.conv2d(converted_extract)
+ return False
+
return [
- ("ethos-n.qnn_mul", qnn_mul_pattern(), check_mul),
Review Comment:
Do the following mul-transforms cover all cases? If not, should the
standalone pattern still be listed in addition to the mul2req / mul2depthwise?
##########
python/tvm/relay/op/contrib/ethosn.py:
##########
@@ -328,19 +338,40 @@ def check_add(extract):
"""Check if an addition is supported by Ethos-N."""
if not ethosn_available():
return False
- # Do not support scalar constants for now
- check_scalar = lambda i: isinstance(i, tvm.relay.Constant) and
len(i.data.shape) == 0
- if check_scalar(extract.args[0]) or check_scalar(extract.args[1]):
- return False
- inputs = extract.args[0:2]
- if any([isinstance(i, tvm.relay.Constant) for i in inputs]):
- extract = _ethosn.ConvertQnnAdd(extract)
- return _ethosn.conv2d(extract)
return _ethosn.addition(extract)
+ def check_add_to_reinterpret_quantize(extract):
+ """Check if addition can be converted to a reinterpret quantize
operation."""
+ if not ethosn_available():
+ return False
+ converted_extract = _ethosn.ConvertQnnAddToReinterpretQuantize(extract)
+ if converted_extract:
+ return _ethosn.reinterpret_quantize(converted_extract)
+ return False
+
+ def check_add_to_depthwise(extract):
+ """Check if addition can be converted to a depthwise operation."""
+ if not ethosn_available():
+ return False
+ converted_extract = _ethosn.ConvertQnnAddToDepthwise(extract)
Review Comment:
I think I have heard of some explanation before, but could you please share
why just knowing that the conversion is possible not enough in this function?
Why can't the next step be ignored?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
