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.

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

[ARM]: Implement CPU plugin just-in-time emitter for Erf operation #27499 #28176

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

shivam5522
Copy link
Contributor

Details:

  • Added a jit_erf_emitter derived class in aarch64/jit_eltwise_emitters
  • Created entry Algorithm::EltwiseErf in the get_supported_precisions in nodes/kernels/aarch64
  • Add the EltwiseErf entry in the aarch64 executors supported algorithms

Tickets:

#27499

@shivam5522 shivam5522 requested review from a team as code owners December 21, 2024 05:17
@github-actions github-actions bot added the category: CPU OpenVINO CPU plugin label Dec 21, 2024
@sys-openvino-ci sys-openvino-ci added the ExternalPR External contributor label Dec 21, 2024
const TReg src(in_vec_idxs[0]);
const TReg dst(out_vec_idxs[0]);

h->erf(dst.s, src.s);
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw your comment with asking help about erf impl (but seems like it was deleted).

You can take a look on JIT Emitter for GeluErf for AArch64 platforms. There is Erf impl

Also, we have JIT Emitter for Erf for x64 platforms. Please take a look - it might be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I checked out the ERF approximation and also the ERF documentation in general. I assume that we have to use the approximation formula to implement ERF. Also can I directly use the same idea from the JIT Emitter for ERF for x64 and then port accordingly for AArch64?
Also, I work on a windows laptop, are there any steps to test this on windows? Sorry for the dumb doubts

Copy link
Contributor

@a-sidorova a-sidorova Dec 23, 2024

Choose a reason for hiding this comment

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

Also can I directly use the same idea from the JIT Emitter for ERF for x64 and then port accordingly for AArch64?

For sure! It makes sense 😃

Also, I work on a windows laptop, are there any steps to test this on windows? Sorry for the dumb doubts

I'd assume that your laptop is on x64 arch (intel or amd). In this case you need emulator to launch programs for arm64. But to be honest, I don't know any good options to do it on windows.
I know only one option with WSL - there you can use cross-compilation and launch programs with QEMU.

Sorry for the dumb doubts

No worries, everything is ok! 👍🏼

@a-sidorova a-sidorova self-assigned this Dec 23, 2024
@a-sidorova a-sidorova added this to the 2025.0 milestone Dec 23, 2024
@a-sidorova a-sidorova added the platform: arm OpenVINO on ARM / ARM64 label Dec 23, 2024
@shivam5522
Copy link
Contributor Author

@a-sidorova Could take a look at it once? Thanks

Copy link
Contributor

@a-sidorova a-sidorova left a comment

Choose a reason for hiding this comment

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

Also could you please update test instances? Please take a look at the last paragraph in the section "What needs to be done?" in the ticket #27499.

Then please locally launch tests using the section "Tests".

h->mov(dst.b16, vmm_aux3.b16);
}

void jit_erf_emitter::emit_data() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the method impl of derived class is the same as of base class and this method is virtual in base class - no need to override this method.

However, you use exp_emitter in the impl of erf_emitter. So you need to emit data for exp_emitter in this method too:

void jit_erf_emitter::emit_data() const {
    jit_emitter::emit_data();
    exp_emitter->emit_data();
}

Otherwise, exp_emitter cannot find the constant values in the table.

I believe that if we add exp_emitter->emit_data();, failed GHA jobs with the following error messages will be fixed:

[ RUN      ] smoke_Activation_Basic/ActivationLayerTest.Inference/IS=([])_TS={(1.50)}_TS=()_Erf_constants_value=()_netPRC=f32_trgDev=CPU
bad err=4 in Xbyak::Error

MEM_USAGE=1972392KB
src/tests/functional/shared_test_classes/src/base/ov_subgraph.cpp:97: Failure
Exception from src/inference/src/cpp/core.cpp:109:
Exception from src/inference/src/dev/plugin.cpp:53:
label is not found


[  FAILED  ] smoke_Activation_Basic/ActivationLayerTest.Inference/IS=([])_TS={(1.50)}_TS=()_Erf_constants_value=()_netPRC=f32_trgDev=CPU, where GetParam() = ((26, {}), f32, ({ ({}, { { 1, 50 } }) }, {}), "CPU") (46 ms)

@shivam5522
Copy link
Contributor Author

Sure adding the tests right now

@shivam5522
Copy link
Contributor Author

@a-sidorova Just to be sure, I need to update the SetUp() to use the ERF operation instead of the current GELU ERF operation right?

@a-sidorova
Copy link
Contributor

@a-sidorova Just to be sure, I need to update the SetUp() to use the ERF operation instead of the current GELU ERF operation right?

You need to add a Erf as a new operation for testing. GeluErf is another operation and should be validated too. So please add Erf in activationTypes() list.

@shivam5522
Copy link
Contributor Author

Other than that, could you review the changes once and see if they are correct?

@a-sidorova
Copy link
Contributor

a-sidorova commented Dec 30, 2024

Other than that, could you review the changes once and see if they are correct?

I suggest to locally validate the implementation launching tests for Erf.
And I think there will be accuracy problems. Because previously we discussed that you can port JIT emitter impl from x64 to aarch64. However, I don't see polynomial calculation as it's done in x64 impl. Please pay attention on this part. Probably, this article also will help you to implement the emitter algorithm 😊

@shivam5522
Copy link
Contributor Author

Sure sorry about that, are there any docs that I can follow for testing, mainly I cannot understand the activation.cpp and how to test my impl? So mainly any docs that I can follow to facilitate testing?
Thanks.

@a-sidorova
Copy link
Contributor

a-sidorova commented Jan 3, 2025

Sure sorry about that, are there any docs that I can follow for testing, mainly I cannot understand the activation.cpp and how to test my impl? So mainly any docs that I can follow to facilitate testing? Thanks.

To implement tests, you need to take a look at this part of the issue description.
image
This paragraph contains links to the code parts which should be updated

How to test:
image

These paragraphs should be enough to test the implementation. If you have specific questions, feel free to ask them 😊

Also there is PR which added tests for jit_ceiling_emitter : #27527. It also might be useful for you.

@a-sidorova a-sidorova modified the milestones: 2025.0, 2025.1 Jan 15, 2025
@a-sidorova
Copy link
Contributor

@shivam5522 Hello! How are things going with implementation and testing? Could you tell us please if you need our help? 😊

Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Feb 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: CPU OpenVINO CPU plugin ExternalPR External contributor platform: arm OpenVINO on ARM / ARM64 Stale
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 Erf operation
3 participants