Skip to content

Conversation

@awayzjj
Copy link
Contributor

@awayzjj awayzjj commented Jun 26, 2024

Closes #24416

Details:

  • item1
  • ...

Tickets:

@awayzjj awayzjj requested review from a team as code owners June 26, 2024 15:28
@github-actions github-actions bot added category: IE Tests OpenVINO Test: plugins and common category: CPU OpenVINO CPU plugin labels Jun 26, 2024
@sys-openvino-ci sys-openvino-ci added the ExternalPR External contributor label Jun 26, 2024
@awayzjj
Copy link
Contributor Author

awayzjj commented Jun 26, 2024

@eshoguli @dmitry-gorokhov Hi, I have implemented this new emitter. Could you please help me review the PR and trigger the CI? Thank you very much!

@awayzjj
Copy link
Contributor Author

awayzjj commented Jun 27, 2024

Hello @dmitry-gorokhov @eshoguli , just a gentle reminder to review this PR when you have time.

@awayzjj
Copy link
Contributor Author

awayzjj commented Jun 28, 2024

@mlukasze Hi, could you please help me trigger the CI?

@mlukasze
Copy link
Contributor

build_jenkins

@awayzjj
Copy link
Contributor Author

awayzjj commented Jun 28, 2024

Hello @dmitry-gorokhov @eshoguli @mlukasze Some tests failed :(

The visible failed test is about JAX FE which seems not related with my PR ?
Could you please help me check other failed tests?

@awayzjj
Copy link
Contributor Author

awayzjj commented Jul 1, 2024

@dmitry-gorokhov @eshoguli Gentle ping.

@awayzjj
Copy link
Contributor Author

awayzjj commented Jul 3, 2024

@dmitry-gorokhov @eshoguli Just a friendly reminder for help when you have time. I want to finish this emitter and move to another!

@a-sidorova
Copy link
Contributor

a-sidorova commented Jul 3, 2024

@awayzjj there are two failed jobs which are not affected by your changes.

I see that at the moment the both of them are green on the master branch, so I suggest to just rebase your branch to fix CI.

@awayzjj awayzjj force-pushed the is_finite_emitter branch from 68839ad to 2e11f21 Compare July 3, 2024 07:03
@awayzjj
Copy link
Contributor Author

awayzjj commented Jul 3, 2024

@a-sidorova Thank you so much! I have rebased my branch, Could you please help me trigger the CI?

@mlukasze
Copy link
Contributor

mlukasze commented Jul 3, 2024

build_jenkins

@awayzjj
Copy link
Contributor Author

awayzjj commented Jul 3, 2024

@a-sidorova Hi, there are 2 jobs that have been hanging for a long time, and 3 jobs have failed. Could you please provide some suggestions on how to address these issues? Thank you very much!

@a-sidorova
Copy link
Contributor

a-sidorova commented Jul 4, 2024

@awayzjj hi! To fix GPU jobs, you can rebase to the latest master to get the commit #25355 which fixes the failed test. As for 2 hanging GHA jobs, there was a queue. I hope rebasing will help you 😃

@awayzjj awayzjj force-pushed the is_finite_emitter branch from 2e11f21 to 686d9ae Compare July 4, 2024 05:29
@awayzjj
Copy link
Contributor Author

awayzjj commented Jul 4, 2024

@a-sidorova Thank you so much! I have rebased my branch, Could you please help me trigger the CI again?

@a-sidorova
Copy link
Contributor

build_jenkins

@awayzjj
Copy link
Contributor Author

awayzjj commented Jul 4, 2024

@a-sidorova @mlukasze Wow, the CI is full green. Can you merge the PR? Thank you very much!

Copy link

@eshoguli eshoguli left a comment

Choose a reason for hiding this comment

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

New emitter handles special values: NaN and Infinity, but tests input tensor generation was not changed. Please, update ActivationLayerCPUTest::generate_inputs to have NaN and Infinity values in test input tensor.
Note, please:

  1. there are several types of NaN values - use different, please,
  2. there is an example for Sign activation here. You can do something similar but use more then one value, please.

@awayzjj awayzjj force-pushed the is_finite_emitter branch 2 times, most recently from 2958716 to a5c3b10 Compare July 8, 2024 16:33
@awayzjj
Copy link
Contributor Author

awayzjj commented Jul 8, 2024

@eshoguli Hi, thank you very much for your review! I have incorporated your suggestions, could you please review the PR again?

@mlukasze mlukasze requested a review from eshoguli July 9, 2024 04:47
@mlukasze
Copy link
Contributor

mlukasze commented Jul 9, 2024

build_jenkins

static_cast<float*>(tensor.data())[3] = std::tgamma(-1); // nan
static_cast<float*>(tensor.data())[4] = std::log(-1); // nan
static_cast<float*>(tensor.data())[5] = std::sqrt(-1); // -nan
static_cast<float*>(tensor.data())[6] = 0 / 0.0; // -nan

Choose a reason for hiding this comment

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

@awayzjj This line leads to failed Win build on CI:
"openvino/src/plugins/intel_cpu/tests/functional/custom/single_layer_tests/classes/activation.cpp(105): error C2124: divide or mod by zero"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dmitry-gorokhov Thank you! But I do not have much experirence on windows. Should I just remove the line 105?

Copy link

@dmitry-gorokhov dmitry-gorokhov Jul 9, 2024

Choose a reason for hiding this comment

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

You are implementing isFinite operation. So I believe it is enough to have 4 specific values: nan, -nan, inf, -inf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I will fix it later today!

@dmitry-gorokhov
Copy link

build_jenkins

@dmitry-gorokhov
Copy link

@awayzjj Could you please rebase you PR on latest master state? This is required to make CI green.

@awayzjj awayzjj force-pushed the is_finite_emitter branch from 166e18f to 2e48dbf Compare July 11, 2024 13:39
@dmitry-gorokhov
Copy link

build_jenkins

auto-merge was automatically disabled July 11, 2024 14:44

Head branch was pushed to by a user without write access

@awayzjj awayzjj force-pushed the is_finite_emitter branch from 2e48dbf to 0b2b2e9 Compare July 11, 2024 14:44
@awayzjj
Copy link
Contributor Author

awayzjj commented Jul 11, 2024

@dmitry-gorokhov Hi, I forgot to update the master before rebase, could you please help me trigger the CI again? Thank you very much!

@mg-intel
Copy link

build_jenkins

@awayzjj
Copy link
Contributor Author

awayzjj commented Jul 12, 2024

@dmitry-gorokhov @eshoguli Hi, the CI is still not full green after rebasing :(

@dmitry-gorokhov dmitry-gorokhov added this pull request to the merge queue Jul 12, 2024
@dmitry-gorokhov
Copy link

@awayzjj CI is green now. Added in merge queue.
Great job!

@awayzjj
Copy link
Contributor Author

awayzjj commented Jul 12, 2024

@dmitry-gorokhov Thank you so much for your patience and help!

I have only one emitter to implement! Before starting, I have one question: how much performance improvements in terms of speed and throughput can we expect from JIT? Are there approximate numbers you can share?

Merged via the queue into openvinotoolkit:master with commit 2bd91ee Jul 12, 2024
@mlukasze mlukasze added this to the 2024.4 milestone Jul 12, 2024
@dmitry-gorokhov
Copy link

@awayzjj

I have only one emitter to implement!

We have two more that need contributors. Feel free to take :)
image

As for performance improvements it depends on the particular operation. As you know except JIT we have 2 backends ACL and reference impls.
JIT usually delivers 10-50% better perf than ACL. Comparing with reference the difference is much more noticable (since reference impl is very slow itself): 10-100x faster.

@awayzjj
Copy link
Contributor Author

awayzjj commented Jul 12, 2024

@dmitry-gorokhov I've taken these two issues and will work on resolving them as quickly as possible. I hope that in the future, you can release slightly different issues, as I want to become more familiar with our ARM backend! :)

spran180 pushed a commit to spran180/openvino that referenced this pull request Jul 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: CPU OpenVINO CPU plugin category: IE Tests OpenVINO Test: plugins and common ExternalPR External contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Good First Issue] [ARM]: Implement CPU plugin just-in-time emitter for IsFinite operation

7 participants