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

Aryan softplus #24378

Closed
wants to merge 10 commits into from
Closed

Aryan softplus #24378

wants to merge 10 commits into from

Conversation

Aryan8912
Copy link
Contributor

close : #24109

@Aryan8912 Aryan8912 requested review from a team as code owners May 6, 2024 09:28
@github-actions github-actions bot added the category: CPU OpenVINO CPU plugin label May 6, 2024
@sys-openvino-ci sys-openvino-ci added the ExternalPR External contributor label May 6, 2024
@dmitry-gorokhov dmitry-gorokhov added this to the 2024.2 milestone May 6, 2024
@eshoguli
Copy link
Contributor

eshoguli commented May 7, 2024

You implemented JIT emitter: good start, thanks! But how did you test it?

I would like to suggest next implementation steps. Explore existing PR from Example Pull Requests section here: #24109, please.

  1. Update tests to be sure that your implementation is used. Tests should fail: acl_f32 primitive is used.
  2. Update JIT kernel. Tests should fail: expected and reference tensors comparation fails.
  3. Finish JIT emitter. Tests should pass.

Let me know if you still have any questions.

@dmitry-gorokhov dmitry-gorokhov added the platform: arm OpenVINO on ARM / ARM64 label May 9, 2024
@Aryan8912
Copy link
Contributor Author

Hello, sir I do not understand the acl_f32 primitive is used. and also how should Finish JIT emitter

@eshoguli
Copy link
Contributor

Hello, sir I do not understand the acl_f32 primitive is used. and also how should Finish JIT emitter

Hi @Aryan8912, you can use Sigmoid implementation as example: #23065. The line where Sigmoid is added in Executer: https://github.com/openvinotoolkit/openvino/pull/23065/files#diff-ce3f79fe72be04c8660d4f853b11227473660eb109c3fc984c6ff4d2b4d97738R32.

Don't ignore tests, please. Let me know if you still have any questions.

eshoguli
eshoguli reviewed May 16, 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.

good start, some comments

src/plugins/intel_cpu/src/emitters/plugin/aarch64/jit_eltwise_emitters.cpp Outdated Show resolved Hide resolved
src/plugins/intel_cpu/src/emitters/plugin/aarch64/jit_eltwise_emitters.hpp Outdated Show resolved Hide resolved
src/plugins/intel_cpu/src/emitters/plugin/aarch64/jit_eltwise_emitters.cpp Outdated Show resolved Hide resolved
@dmitry-gorokhov dmitry-gorokhov modified the milestones: 2024.2, 2024.3 May 23, 2024
eshoguli
eshoguli reviewed May 24, 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.

Comment about implementation

src/plugins/intel_cpu/src/emitters/plugin/aarch64/jit_eltwise_emitters.cpp
Comment on lines +932 to +943
h->vldr(src.d, v0.d);
h->vmul(dst.d, v0.d, v0.d);
h->vcvt(dst.s, dst.d);
h->vfma(dst.s, v0.s, vptrue, v1.s, dst.s);
h->vmaxnm(dst.s, dst.s, vmmin.s);
h->vcvt(dst.d, dst.s);
h->vexp2(dst.d, dst.d);
h->vadd(dst.d, dst.d, vone.d);
h->vrcps(dst.d, dst.d);
h->vrsqrts(dst.d, dst.d);
h->vadd(dst.d, dst.d, vzero.d);
h->vst1(dst.d, ptr[dst_reg]);
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. You don't need to load anything in emitter. By the main concept: everything has been loaded in vector registers already, use registers.
  2. Inside emitter you can use only in_vec_idxs, out_vec_idxs and aux_vec_idxs vector registers (their indexes).
  3. In float implementation use word only.
  4. Use instrucations from SIMD&FP Instructions from ARM64 instruction set. We don't support ARM32 in OpenVINO snippets.

Note, please, you can use PRs as example:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @eshoguli please tell me more details about this, I am busy with other things

@mlukasze
Copy link
Contributor

hey @Aryan8912, will you have a time to finish this?

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 4, 2024

This PR will be closed in a week because of 2 weeks of no activity.

@github-actions github-actions bot added the Stale label Jul 4, 2024
@mg-intel mg-intel removed the Stale label Jul 4, 2024
@mg-intel mg-intel removed this from the 2024.3 milestone Jul 16, 2024
@mg-intel mg-intel added this to the 2024.4 milestone Jul 16, 2024
@mlukasze mlukasze closed this 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 left review comments

Assignees

@eshoguli eshoguli

Labels
category: CPU OpenVINO CPU plugin ExternalPR External contributor platform: arm OpenVINO on ARM / ARM64
Projects
None yet
Milestone
2024.4
Development

Successfully merging this pull request may close these issues.

[Good First Issue] [ARM]: Implement CPU plugin just-in-time emitter for SoftPlus operation
6 participants
@Aryan8912 @eshoguli @mlukasze @dmitry-gorokhov @sys-openvino-ci @mg-intel