-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add EQUAL_ELTWISE support for eltwise test and kernel #30877
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
base: master
Are you sure you want to change the base?
Conversation
src/plugins/intel_cpu/src/nodes/kernels/riscv64/jit_uni_eltwise_generic.cpp
Outdated
Show resolved
Hide resolved
src/plugins/intel_cpu/tests/functional/shared_tests_instances/single_layer_tests/eltwise.cpp
Outdated
Show resolved
Hide resolved
src/plugins/intel_cpu/src/emitters/plugin/riscv64/jit_eltwise_emitters.cpp
Outdated
Show resolved
Hide resolved
src/plugins/intel_cpu/src/emitters/plugin/riscv64/jit_eltwise_emitters.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completed.
Hi @a-sidorova . I made the changes as you suggested. Could you please check? I was able to get all the test cases passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@madhurthareja please apply the comments, build the tests and launch them. Please check that all of them are passed and send the screenshot with them 😊
@@ -80,7 +80,7 @@ std::vector<EltwiseTypes> eltwise_op_types = { | |||
EltwiseTypes::FLOOR_MOD, | |||
EltwiseTypes::SQUARED_DIFF, | |||
EltwiseTypes::POWER, | |||
EltwiseTypes::MOD | |||
EltwiseTypes::MOD, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: please remove extra comma
@@ -593,7 +594,8 @@ std::set<std::vector<element::Type>> eltwise_precision_helper::get_supported_pre | |||
OV_CASE(Algorithm::EltwiseRelu, jit_relu_emitter), | |||
OV_CASE(Algorithm::EltwiseSigmoid, jit_sigmoid_emitter), | |||
OV_CASE(Algorithm::EltwiseSqrt, jit_sqrt_emitter), | |||
OV_CASE(Algorithm::EltwiseSubtract, jit_subtract_emitter)); | |||
OV_CASE(Algorithm::EltwiseSubtract, jit_subtract_emitter), | |||
OV_CASE(Algorithm::EltwiseEqual, jit_equal_emitter)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You moved EltwiseEqual
to right place only in first registration list, what's about this one? :) Could you please fix it here too?
size_t jit_equal_emitter::aux_fp_gprs_count() const { | ||
return 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see definition of the method aux_fp_gprs_count()
here but I don't see the method declaration in the header. Please add it to the header.
jit_equal_emitter::jit_equal_emitter(ov::intel_cpu::riscv64::jit_generator* host, | ||
ov::intel_cpu::riscv64::cpu_isa_t host_isa, | ||
const std::shared_ptr<ov::Node>& node) | ||
: jit_emitter(host, host_isa, get_arithmetic_binary_exec_precision(node)) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you use constants from the table (register_table_entries()
), you need to call prepare_table()
in constructors. Please see example in jit_exp_emitter
.
h->vmv_v_x(dst, zero); // set dst to 0 | ||
h->vfeq_vv(mask_vreg(), src0, src1); // compare, result in mask | ||
h->vfadd_vf(dst, dst, one, VM::masked); // set 1.0 where mask is true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove extra }
Yes, I'll do it. Thanks for your guidance. I'm a little naive in OC contributions, but loving them now with the help you and fellow contributors are providing. |
In general you don't need to launch all existing tests in OV - only
I believe that some tests might be failed due to missed packages/models/installed frontends. This is OK. Please pay attention only on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@madhurthareja your changes look better now, thanks! I left a few comments + I launched CI jobs. Please take a look at the failed code-style jobs and apply suggested changes 😃
src/tests/test_utils/common_test_utils/include/common_test_utils/test_enums.hpp
Outdated
Show resolved
Hide resolved
src/tests/test_utils/common_test_utils/src/node_builders/eltwise.cpp
Outdated
Show resolved
Hide resolved
I made the required changes, but the issues with CI jobs seems to be regarding some permissions. Would I have to set it up or would that be done from the admin side? |
Anybody can open reports of GHA jobs. Please take a look at them and apply suggestions from this job (I see that you haven't made it yet):
As for launching CI job, only Intel-workers can do it (like me). So when you apply code-style suggestions, please let me know - I will relaunch CI 😊 |
@a-sidorova I checked the reports and found that: The GitHub Actions token used by the workflow does not have permission to post review comments on the PR. Diff in eltwise.cpp: This is a no-op change (removing and re-adding the same line). It is likely a formatting artifact and does not affect functionality. I'm not sure why is it failing. Is there due to formatting issues? Maybe some spaces or extra tabs? Please let me know if I'm unable to understand the issue. I'll retry to understand them. |
Hi!
I think this is internal error and it come up only when there are problems with code-style. So no need to pay attention on it :)
Code-style suggester suggests to swap lines with includes - please note that the difference between lines - number of lines. But this error is outdated - you reverted changes from the file |
Details:
Implement CPU plugin just-in-time emitter for Equal operation
Tickets:
30257