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

Implement IsInf JIT Emitter for ARM64 SIMD in OpenVINO #24471

Merged
merged 2 commits into from
Jun 10, 2024
Merged

Implement IsInf JIT Emitter for ARM64 SIMD in OpenVINO #24471

merged 2 commits into from
Jun 10, 2024

Conversation

inbasperu
Copy link
Contributor

Hello maintainers,

I've implemented the IsInf JIT emitter for the ARM64 SIMD platform, as outlined in the OpenVINO CPU plugin JIT emitters documentation.

This PR addresses #24419.

Please let me know if any further adjustments are needed.
Thank you!

@inbasperu inbasperu requested review from a team as code owners May 11, 2024 05:19
@github-actions github-actions bot added the category: CPU OpenVINO CPU plugin label May 11, 2024
@sys-openvino-ci sys-openvino-ci added the ExternalPR External contributor label May 11, 2024
@inbasperu inbasperu mentioned this pull request May 11, 2024
Closed
@ilya-lavrenov ilya-lavrenov linked an issue May 11, 2024 that may be closed by this pull request
[Good First Issue] [ARM]: Implement CPU plugin just-in-time emitter for IsInf operation #24419
Closed
@ilya-lavrenov ilya-lavrenov added the platform: arm OpenVINO on ARM / ARM64 label May 11, 2024
eshoguli
eshoguli reviewed May 13, 2024
View reviewed changes
Copy link
Contributor

@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.

Thanks, good start, there are minor comments.
Don't forget about the tests, please.

src/plugins/intel_cpu/src/emitters/plugin/aarch64/jit_eltwise_emitters.cpp Outdated

} else {
// If neither positive nor negative infinity detection is enabled, load 'dst' with zeros.
h->ld1r(dst.s, table_val2("zero"));
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need store zero, use eor instruction instead

src/plugins/intel_cpu/src/emitters/plugin/aarch64/jit_eltwise_emitters.hpp Outdated
Comment on lines 195 to 196
bool detect_negative = true,
bool detect_positive = true);
Copy link
Contributor

Choose a reason for hiding this comment

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

const bool detect_negative = true,
const bool detect_positive = true);

@inbasperu
Copy link
Contributor Author

Thanks, good start, there are minor comments. Don't forget about the tests, please.

Thank you @eshoguli for the comments. I will make these changes.
I had some questions about the tests. In the description of the issue here, it is mentioned that we should add tests for the supported operands in the respective element-wise or activation files. However, I am not sure how to proceed with the ComparisonTypes. I did look at a similar example, the equal JIT emitter, in this pull request, but couldn't find how to add tests. Could you please help me with this?

@eshoguli
Copy link
Contributor

Thanks, good start, there are minor comments. Don't forget about the tests, please.

Thank you @eshoguli for the comments. I will make these changes. I had some questions about the tests. In the description of the issue here, it is mentioned that we should add tests for the supported operands in the respective element-wise or activation files. However, I am not sure how to proceed with the ComparisonTypes. I did look at a similar example, the equal JIT emitter, in this pull request, but couldn't find how to add tests. Could you please help me with this?

Use this commit eshoguli@1e27b76 for Maximum & Minimum operations as example how to easilly add new operation to existing eltwise test. Let me know if you still have any questions.

@inbasperu inbasperu requested review from a team as code owners May 19, 2024 08:23
@github-actions github-actions bot added the category: IE Tests OpenVINO Test: plugins and common label May 19, 2024
@inbasperu
Copy link
Contributor Author

Thanks, good start, there are minor comments. Don't forget about the tests, please.

Thank you @eshoguli for the comments. I will make these changes. I had some questions about the tests. In the description of the issue here, it is mentioned that we should add tests for the supported operands in the respective element-wise or activation files. However, I am not sure how to proceed with the ComparisonTypes. I did look at a similar example, the equal JIT emitter, in this pull request, but couldn't find how to add tests. Could you please help me with this?

Use this commit eshoguli@1e27b76 for Maximum & Minimum operations as example how to easilly add new operation to existing eltwise test. Let me know if you still have any questions.

Hey @eshoguli,
I have incorporated the code review feedback and added the IsInf operation to the existing element-wise tests. However, when I run the command ./bin/arm64/Release/ov_cpu_func_tests --gtest_filter="*smoke*Eltwise*", 76 tests failed. I'm not sure how to proceed further. Could you please guide me?

image

@eshoguli
Copy link
Contributor

Hey @eshoguli, I have incorporated the code review feedback and added the IsInf operation to the existing element-wise tests. However, when I run the command ./bin/arm64/Release/ov_cpu_func_tests --gtest_filter="*smoke*Eltwise*", 76 tests failed. I'm not sure how to proceed further. Could you please guide me?

The fix: inbasperu@94e04bb. I added the commit to your branch. Please, review the commit and rebase your branch to the latest master.

@inbasperu
feat: Add IsInf JIT emitter for ARM64
71bb8c8 
Incorporated code review feedback

Added IsInf operation to the existing element-wise tests

[CPU] [ARM] JIT IsInf: emitter fixes & tests
@inbasperu inbasperu force-pushed the 24419-arm-isinf-emitter branch 2 times, most recently from 5b86067 to 71bb8c8 Compare May 23, 2024 17:22
@eshoguli
Copy link
Contributor

build_jenkins

eshoguli
eshoguli approved these changes May 30, 2024
View reviewed changes
Copy link
Contributor

@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.

Thanks! One minor comment, we are waiting CI completion

@dmitry-gorokhov
Copy link
Contributor

build_jenkins

@dmitry-gorokhov dmitry-gorokhov added this pull request to the merge queue Jun 10, 2024
Merged via the queue into openvinotoolkit:master with commit 9465604 Jun 10, 2024
119 checks passed
@dmitry-gorokhov
Copy link
Contributor

@inbasperu PR is merged into master branch, Thanks for the contribution!

alexandruenache1111 pushed a commit to alexandruenache1111/openvino that referenced this pull request Jun 13, 2024
@inbasperu @alexandruenache1111
Implement IsInf JIT Emitter for ARM64 SIMD in OpenVINO (openvinotoolk…
9080e76 
…it#24471)

Hello maintainers,

I've implemented the IsInf JIT emitter for the ARM64 SIMD platform, as
outlined in the OpenVINO CPU plugin JIT emitters documentation.

This PR addresses
[openvinotoolkit#24419](openvinotoolkit#24419).

Please let me know if any further adjustments are needed.
Thank you!
@inbasperu inbasperu deleted the 24419-arm-isinf-emitter branch June 26, 2024 15:25
allnes pushed a commit to allnes/openvino that referenced this pull request Jun 27, 2024
@inbasperu @allnes
Implement IsInf JIT Emitter for ARM64 SIMD in OpenVINO (openvinotoolk…
654b188 
…it#24471)

Hello maintainers,

I've implemented the IsInf JIT emitter for the ARM64 SIMD platform, as
outlined in the OpenVINO CPU plugin JIT emitters documentation.

This PR addresses
[openvinotoolkit#24419](openvinotoolkit#24419).

Please let me know if any further adjustments are needed.
Thank you!
@mlukasze mlukasze added this to the 2024.3 milestone Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eshoguli eshoguli eshoguli approved these changes

Assignees

@eshoguli eshoguli

Labels
category: CPU OpenVINO CPU plugin category: IE Tests OpenVINO Test: plugins and common ExternalPR External contributor platform: arm OpenVINO on ARM / ARM64
Projects
None yet
Milestone
2024.3
Development

Successfully merging this pull request may close these issues.

[Good First Issue] [ARM]: Implement CPU plugin just-in-time emitter for IsInf operation
6 participants
@inbasperu @eshoguli @dmitry-gorokhov @ilya-lavrenov @mlukasze @sys-openvino-ci