Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Reduce node checks avx2 accessability for jit impl #5658

Closed
wants to merge 1 commit into from
Closed

Reduce node checks avx2 accessability for jit impl #5658

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion inference-engine/src/mkldnn_plugin/nodes/mkldnn_reduce_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1427,7 +1427,7 @@ void MKLDNNReduceNode::initSupportedPrimitiveDescriptors() {
Precision inputPrecision = getOriginalInputPrecisionAtPort(REDUCE_DATA);
Precision outputPrecision = getOriginalOutputPrecisionAtPort(0);

jit_mode = (mayiuse(cpu::x64::sse41)) && getParentEdgeAt(REDUCE_DATA)->getDims().ndims() <= 5 &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the machine supports sse41 while not supporting avx2? This case jit_mode gets false. sse41 can't be utilized.
I think the original code here is right. Please also refer to the following comment: #580 (comment)
Thanks Yury!

Copy link
Contributor Author

@yury-intel yury-intel May 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @xuchen-intel, I agree, that such solution seems to be a workaround, but we still have to fix original bug in sse4.2 case. I found it located in reduce_main() and reduce_tail() functions, but still cannot found the separate 'illegal instruction'. May be, it doesn't connected with specific operator itself. Have you any ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't reproduce this problem with CPU-test. It passes on no-AVX2 machine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Yury! If you've already been working on the JIT details, maybe you can try to reproduce this problem by changing the following code.

openvino/inference-engine/src/mkldnn_plugin/nodes/mkldnn_reduce_node.cpp

Line 1528 in 76131da

if (mayiuse(cpu::x64::avx512_common)) {

Remove the condition branch of avx512 and avx2, so that it will be forced to utilize sse4.2 instruction.
Anyway, I'm OK with the workaround. And you can always to assign the issue to me, if you come across any problem.

Copy link
Contributor Author

@yury-intel yury-intel May 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to remove those conditions. It doesn't helps, so I have had to reproduce the problem with no-AVX machine via benchmark_app and GNMT model. Solution with additional (mayiuse(cpu::x64::avx2)) works good. I tried to reproduce it in special test for reduce_node to reassign it reasonably, but without result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, thanks for advice. I'll consult with our team about acceptability of workaround in given case.

jit_mode = (mayiuse(cpu::x64::sse41)) && (mayiuse(cpu::x64::avx2)) && getParentEdgeAt(REDUCE_DATA)->getDims().ndims() <= 5 &&
std::find(std::begin(supportedPrecisions), std::end(supportedPrecisions), inputPrecision) != std::end(supportedPrecisions) &&
std::find(std::begin(supportedPrecisions), std::end(supportedPrecisions), outputPrecision) != std::end(supportedPrecisions);

Expand Down