-
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
[microNPU] Mean legalization support #9576
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.
Great work @lhutton1, mean is a tough one! Just some clarifying questions...
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 @ekalda for the review :)
10a2b2f
to
47daacc
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.
LGTM! :)
5fa39af
to
66ccdc1
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.
@lhutton1 please do change the test post using target hooks the device runtime module is imported inside the host runtime module. You should be able to see examples in other tests.
Supports legalizing a Relay mean operation to an equivalent series of NPU operations. Mean can be legalized given one of three cases: - Case 1 (axis == [1, 2] and keepsdims == True): depthwise_conv2d + binary_elementwise - Case 2 (ifm qparams == ofm qparams): pooling - Case 3 (else): depthwise_conv2d Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com> Change-Id: Iefbed5d9dc6da4e349127ea70fd1bf56b90ef4a3
Change-Id: I3c82f9b492a58082c98c200d42d5413451740504
Change-Id: Idcab5f71f3c5a971831b96fe8f9a5c8c73b267da
66ccdc1
to
18ee95b
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.
LGTM.
Few nits -- nothing to block this from going in. I ll trust you could do address them (ones you agree) in a follow up ?
) | ||
|
||
n = int(filter_height * filter_width) | ||
eps = 1 / (256 * (n + 1)) if n % 2 == 0 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.
It might be worth using variables for numbers to say what they mean.
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.
Ack
@@ -166,11 +181,11 @@ bool EthosuBinaryElementwiseRel(const Array<Type>& types, int num_inputs, const | |||
|
|||
if (operator_type == "ADD" || operator_type == "SUB" || operator_type == "MUL") { | |||
if (ifm_dtype != DataType::UInt(8) && ifm_dtype != DataType::Int(8) && | |||
ifm_dtype != DataType::Int(32)) { | |||
ifm_dtype != DataType::Int(16) && ifm_dtype != DataType::Int(32)) { |
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.
nit : It might be worth putting these into a disallowed list and check the presense in the list ?
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.
Ack
@@ -132,6 +136,18 @@ bool EthosuDepthwiseConv2DRel(const Array<Type>& types, int num_inputs, const At | |||
const auto* param = attrs.as<EthosuDepthwiseConv2DAttrs>(); | |||
ICHECK(param != nullptr) << "EthosuDepthwiseConv2DAttrs cannot be nullptr."; | |||
|
|||
DataType ofm_dtype; | |||
|
|||
if (param->ofm_dtype == "int8") { |
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.
It might be clearer if we use a dictionary here
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.
Ack
@@ -156,6 +172,15 @@ bool EthosuDepthwiseConv2DRel(const Array<Type>& types, int num_inputs, const At | |||
return false; | |||
} | |||
|
|||
if (ofm_dtype != DataType::UInt(8) && ofm_dtype != DataType::Int(8) && |
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.
nit : again a disallowed list might helps things to be more clearer
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.
Ack
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 @manupa-arm, I'll address these in a follow up
) | ||
|
||
n = int(filter_height * filter_width) | ||
eps = 1 / (256 * (n + 1)) if n % 2 == 0 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.
Ack
@@ -166,11 +181,11 @@ bool EthosuBinaryElementwiseRel(const Array<Type>& types, int num_inputs, const | |||
|
|||
if (operator_type == "ADD" || operator_type == "SUB" || operator_type == "MUL") { | |||
if (ifm_dtype != DataType::UInt(8) && ifm_dtype != DataType::Int(8) && | |||
ifm_dtype != DataType::Int(32)) { | |||
ifm_dtype != DataType::Int(16) && ifm_dtype != DataType::Int(32)) { |
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.
Ack
@@ -132,6 +136,18 @@ bool EthosuDepthwiseConv2DRel(const Array<Type>& types, int num_inputs, const At | |||
const auto* param = attrs.as<EthosuDepthwiseConv2DAttrs>(); | |||
ICHECK(param != nullptr) << "EthosuDepthwiseConv2DAttrs cannot be nullptr."; | |||
|
|||
DataType ofm_dtype; | |||
|
|||
if (param->ofm_dtype == "int8") { |
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.
Ack
@@ -156,6 +172,15 @@ bool EthosuDepthwiseConv2DRel(const Array<Type>& types, int num_inputs, const At | |||
return false; | |||
} | |||
|
|||
if (ofm_dtype != DataType::UInt(8) && ofm_dtype != DataType::Int(8) && |
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.
Ack
Fixes failing CI since CI on apache#9576 passed before apache#9624 was merged. Change-Id: I44f6aa0abe09d206f3e948e36e9799eaa12b3d14
Supports legalizing a Relay mean operation to an equivalent series of NPU operations. Mean can be legalized given one of three cases: - Case 1 (axis == [1, 2] and keepsdims == True): depthwise_conv2d + binary_elementwise - Case 2 (ifm qparams == ofm qparams): pooling - Case 3 (else): depthwise_conv2d Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com>
Fixes failing CI since CI on apache#9576 passed before apache#9624 was merged. Change-Id: I44f6aa0abe09d206f3e948e36e9799eaa12b3d14
Supports legalizing a Relay mean operation to an equivalent series of NPU operations. Mean can be legalized given one of three cases: - Case 1 (axis == [1, 2] and keepsdims == True): depthwise_conv2d + binary_elementwise - Case 2 (ifm qparams == ofm qparams): pooling - Case 3 (else): depthwise_conv2d Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com>
Fixes failing CI since CI on apache#9576 passed before apache#9624 was merged. Change-Id: I44f6aa0abe09d206f3e948e36e9799eaa12b3d14
Supports legalizing a Relay mean operation to an equivalent series of NPU operations. Mean can be legalized given one of three cases: - Case 1 (axis == [1, 2] and keepsdims == True): depthwise_conv2d + binary_elementwise - Case 2 (ifm qparams == ofm qparams): pooling - Case 3 (else): depthwise_conv2d Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com>
Fixes failing CI since CI on apache#9576 passed before apache#9624 was merged. Change-Id: I44f6aa0abe09d206f3e948e36e9799eaa12b3d14
Supports legalizing a Relay mean operation to an equivalent series of NPU operations. Mean can be legalized given one of three cases: - Case 1 (axis == [1, 2] and keepsdims == True): depthwise_conv2d + binary_elementwise - Case 2 (ifm qparams == ofm qparams): pooling - Case 3 (else): depthwise_conv2d Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com>
Fixes failing CI since CI on apache#9576 passed before apache#9624 was merged. Change-Id: I44f6aa0abe09d206f3e948e36e9799eaa12b3d14
Aims to improve readability, extendibility and error message unification for data type checks across NPU operators. A follow up for the comments in apache#9576. Change-Id: I83fb89a56677003f7abebb7985ad60d92cfa8df1
Aims to improve readability, extendibility and error message unification for data type checks across NPU operators. A follow up for the comments in apache#9576. Change-Id: I83fb89a56677003f7abebb7985ad60d92cfa8df1
Aims to improve readability, extendibility and error message unification for data type checks across NPU operators. A follow up for the comments in apache#9576. Change-Id: I83fb89a56677003f7abebb7985ad60d92cfa8df1
* [microNPU] Refactor type inference data type checks Aims to improve readability, extendibility and error message unification for data type checks across NPU operators. A follow up for the comments in #9576. Change-Id: I83fb89a56677003f7abebb7985ad60d92cfa8df1 * unordered_set -> initializer_list and use new format for upscale check Change-Id: Icf3d68d5cc7d5e1d5af42b1af193db89faea155e * remove unused header and use auto for initializer type Change-Id: I10311b718c3abd0ed75dd88b5ec9de6e0742f047
* [microNPU] Refactor type inference data type checks Aims to improve readability, extendibility and error message unification for data type checks across NPU operators. A follow up for the comments in apache#9576. Change-Id: I83fb89a56677003f7abebb7985ad60d92cfa8df1 * unordered_set -> initializer_list and use new format for upscale check Change-Id: Icf3d68d5cc7d5e1d5af42b1af193db89faea155e * remove unused header and use auto for initializer type Change-Id: I10311b718c3abd0ed75dd88b5ec9de6e0742f047
Supports legalizing a Relay mean operation to an equivalent series of NPU operations. Mean can be legalized given one of three cases: - Case 1 (axis == [1, 2] and keepsdims == True): depthwise_conv2d + binary_elementwise - Case 2 (ifm qparams == ofm qparams): pooling - Case 3 (else): depthwise_conv2d Co-authored-by: Rishabh Jain <rishabh.jain2@arm.com>
Fixes failing CI since CI on apache#9576 passed before apache#9624 was merged. Change-Id: I44f6aa0abe09d206f3e948e36e9799eaa12b3d14
Supports legalizing a Relay mean operation to an equivalent series of NPU operations. Mean can be legalized given one of three cases:
- Case 1 (axis == [1, 2] and keepsdims == True):
depthwise_conv2d + binary_elementwise
- Case 2 (ifm qparams == ofm qparams):
pooling
- Case 3 (else):
depthwise_conv2d
cc @ekalda @NicolaLancellotti @dchauhan-arm @manupa-arm @mbaret @jainris
Co-authored-by: Rishabh Jain rishabh.jain2@arm.com