-
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
MVN_accuracy_fix_on_avx512 #5787
MVN_accuracy_fix_on_avx512 #5787
Conversation
@chenhu-wang what about tests? Why the issue is not reproducible by our test coverage? |
@dmitry-gorokhov ,the bug is in LP case, I8 computing behavior is not consistent between CPU plugin and ngraph reference. You can see [here}(https://github.com/openvinotoolkit/openvino/blob/master/ngraph/core/reference/include/ngraph/runtime/reference/mvn.hpp)the mean, variance and eps are all treated as LP while CPU plugin treated as fp32. So tests will fail when cover I8 precision. I created separate ticket to do this. Local test is done with dump model blob and compare. Thank you. |
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.
@chenhu-wang what about tests? Why the issue is not reproducible by our test coverage?
@dmitry-gorokhov ,the bug is in LP case, I8 computing behavior is not consistent between CPU plugin and ngraph reference. You can see [here}(https://github.com/openvinotoolkit/openvino/blob/master/ngraph/core/reference/include/ngraph/runtime/reference/mvn.hpp)the mean, variance and eps are all treated as LP while CPU plugin treated as fp32. So tests will fail when cover I8 precision. I created separate ticket to do this. Local test is done with dump model blob and compare. Thank you.
@dmitry-gorokhov Partial LP tests(I8 input) is added, because we can fix set execution precision of ngraph reference as fp32, I8 input is converted and executed with fp32 in ngraph. This can cover load int8 to vmm as float. Meanwhile output is also fp32 in ngraph, can not cover I8 output for now. will done in the ticket created.
@@ -703,12 +703,6 @@ void MKLDNNMVNNode::initSupportedPrimitiveDescriptors() { | |||
setPostOps(attr, true); | |||
|
|||
Precision inputPrecision = getOriginalInputPrecisionAtPort(0); | |||
if (getParentEdgeAt(0)->getDims().ndims() < 3 || getParentEdgeAt(0)->getDims().ndims() > 5 |
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.
@dmitry-gorokhov I think this is legacy constraints, so removed, correct me please if I am wrong.
4dd5f5f
to
179ff3f
Compare
@mandrono ,take a look please, fusion condition is extended for the ngraph migration perf regression issue for "brain-tumor-segmentation-0002". |
@dmitry-gorokhov ,all changes is finished, take a look please. |
Details:
Tickets: