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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Implement IsInf JIT Emitter for ARM64 SIMD in OpenVINO #24471

wants to merge 1 commit into from
+156 −2

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
Open
@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
Open
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eshoguli eshoguli eshoguli left review comments

At least 1 approving review is required to merge this pull request.

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
No milestone
Development

Successfully merging this pull request may close these issues.

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