-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[CPU][ARM] Enable NHWC in Reduce #23108
Conversation
There is a theory that SLT tests are passed because of tensor comparison issue. |
#PR23110 is in Merge Queue |
This PR will be closed in a week because of 2 weeks of no activity. |
This reverts commit c547654.
@EgorDuplensky , could you please review? |
d93f189
to
3b6818f
Compare
3b6818f
to
d5614d7
Compare
src/plugins/intel_cpu/src/transformations/cpu_opset/arm/pass/convert_reduce_no_keep_dims.cpp
Outdated
Show resolved
Hide resolved
> | ||
ReduceConvertCPUTestParamsSet; | ||
|
||
class ReduceNoKeepDimsTransformationCPUTest: public testing::WithParamInterface<ReduceConvertCPUTestParamsSet>, |
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.
I think the idea was to write a unit test instead.
Since there is a corresponding single layer test which covers the inference functionality already.
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.
Replaced subgraph test with unit test
src/plugins/intel_cpu/src/transformations/cpu_opset/arm/pass/convert_reduce_no_keep_dims.cpp
Outdated
Show resolved
Hide resolved
* Description: | ||
* ConvertReduceNoKeepDimsBase detects Reduce operations with keepDims = false. | ||
* Such Reduce operation is replaced with Reduce operation with keepDims = true and Squeeze | ||
* which removes undesired dimensions. |
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.
@itikhono Could you please share your opinion regarding the appropriate place for this transformation? We do have an inverted one (where we fuse squeeze into reduce instead) in the common transformations scope. Isn't it better to place this one into the location of the common transformations as well?
I am not proposing to do this in scope of the current PR, but we could do it right after.
@maxnick Could you please take a look |
NHWC was disabled because of accuracy issue: ARM-software/ComputeLibrary#1044 CVS-114403 --------- Co-authored-by: eshoguli <[email protected]>
NHWC was disabled because of accuracy issue: ARM-software/ComputeLibrary#1044
CVS-114403