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

[ARM CPU] Fix AUGRU layer in dien.xml model #21925

Merged
merged 2 commits into from
Mar 14, 2024
Merged

[ARM CPU] Fix AUGRU layer in dien.xml model #21925

merged 2 commits into from
Mar 14, 2024

Conversation

allnes
Copy link
Contributor

@allnes allnes commented Jan 2, 2024 •
edited
Loading

Details:

  • Fixed problem with precision in RNN and TensorIterator layers
  • Corrected shape calculation of current layers in ACL Scheduler

Tickets:

@allnes allnes added category: CPU OpenVINO CPU plugin platform: arm OpenVINO on ARM / ARM64 labels Jan 2, 2024
@allnes allnes added this to the 2024.0 milestone Jan 2, 2024
@allnes allnes requested a review from dmitry-gorokhov January 2, 2024 16:32
@allnes allnes requested review from a team as code owners January 2, 2024 16:32
dmitry-gorokhov
dmitry-gorokhov requested changes Jan 2, 2024
View reviewed changes
src/plugins/intel_cpu/src/nodes/rnn.cpp Show resolved Hide resolved
src/plugins/intel_cpu/src/nodes/tensoriterator.cpp Outdated
@@ -293,7 +293,8 @@ MemoryPtr DynamicBuffer::create_buffer(const dnnl::engine& eng) {
const auto estimated_iters = estimate_iters();
const Shape _shape = Shape({count, static_cast<size_t>(abs_stride * estimated_iters), len/elem_size});
auto _descCreator = BlockedDescCreator::getCommonCreators().at(LayoutType::ncsp);
auto new_buffer_desc = _descCreator->createSharedDesc(from->getDesc().getPrecision(), _shape);
auto prec = from->getDesc().getPrecision() == ov::element::f16 ? ov::element::f32 : from->getDesc().getPrecision();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need this change?
TensorIterator has nothing x64 or ARM specific from the impl perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this change depend on augru rnn layer if we don't set fp32, seg fault is created in loop after rnn layer

Copy link
Contributor

Choose a reason for hiding this comment

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

Is doesn't mean we need to create such kind of WA.
TI should work correctly w/o dependency on parent operation. We need to analyze the issue and find real root-cause

Copy link
Contributor Author

@allnes allnes Feb 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@dmitry-gorokhov its became irrelevant change, dien.xml is worked without tensoriterator changes (all problem was depend on fp16 in rnn and calculate shapes in scheduler)

src/plugins/intel_cpu/src/nodes/executors/acl/acl_ie_scheduler.cpp Show resolved Hide resolved
@github-actions GitHub Actions
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 Jan 25, 2024
@allnes allnes removed the Stale label Jan 29, 2024
@allnes allnes modified the milestones: 2024.0, 2024.1 Feb 16, 2024
@alvoron
Copy link
Contributor

alvoron commented Feb 29, 2024

Although dien model still can't be inferred successfully on ARM:

Check 'element::Type::merge(result_et, get_input_element_type(1), get_input_element_type(2))' failed at src/core/src/op/select.cpp:70:
While validating node 'opset1::Select Select_3046 (opset1::Parameter Parameter_3043[0]:boolean[1,1,10], opset1::Parameter Parameter_3044[0]:f16[1,1,10], opset1::Parameter Parameter_3045[0]:f32[1,1,10]) -> (dynamic[...])' with friendly_name 'Select_3046':
Argument 1 and 2 element types must match.

@allnes can we fix this issue in this PR?
Thank you.

@allnes allnes requested review from a team as code owners March 4, 2024 13:30
@github-actions github-actions bot added category: inference OpenVINO Runtime library - Inference category: Core OpenVINO Core (aka ngraph) category: GPU OpenVINO GPU plugin labels Mar 4, 2024
@github-actions github-actions bot added category: TFL FE OpenVINO TensorFlow Lite FrontEnd category: JS API OpenVino JS API Bindings labels Mar 4, 2024
@allnes allnes removed request for a team March 4, 2024 13:30
@github-actions github-actions bot removed category: inference OpenVINO Runtime library - Inference category: Core OpenVINO Core (aka ngraph) category: GPU OpenVINO GPU plugin category: Python API OpenVINO Python bindings category: CI OpenVINO public CI category: TEMPLATE OpenVINO Template plugin category: TF FE OpenVINO TensorFlow FrontEnd github_actions Pull requests that update GitHub Actions code category: TFL FE OpenVINO TensorFlow Lite FrontEnd category: JS API OpenVino JS API Bindings labels Mar 4, 2024
@dmitry-gorokhov
Copy link
Contributor

Although dien model still can't be inferred successfully on ARM:

Check 'element::Type::merge(result_et, get_input_element_type(1), get_input_element_type(2))' failed at src/core/src/op/select.cpp:70:
While validating node 'opset1::Select Select_3046 (opset1::Parameter Parameter_3043[0]:boolean[1,1,10], opset1::Parameter Parameter_3044[0]:f16[1,1,10], opset1::Parameter Parameter_3045[0]:f32[1,1,10]) -> (dynamic[...])' with friendly_name 'Select_3046':
Argument 1 and 2 element types must match.

@allnes can we fix this issue in this PR? Thank you.

@allnes @alvoron So does Dien works successfuly now or we still have an issue?

@allnes
Copy link
Contributor Author

allnes commented Mar 6, 2024

Although dien model still can't be inferred successfully on ARM:

Check 'element::Type::merge(result_et, get_input_element_type(1), get_input_element_type(2))' failed at src/core/src/op/select.cpp:70:
While validating node 'opset1::Select Select_3046 (opset1::Parameter Parameter_3043[0]:boolean[1,1,10], opset1::Parameter Parameter_3044[0]:f16[1,1,10], opset1::Parameter Parameter_3045[0]:f32[1,1,10]) -> (dynamic[...])' with friendly_name 'Select_3046':
Argument 1 and 2 element types must match.

@allnes can we fix this issue in this PR? Thank you.

@allnes @alvoron So does Dien works successfuly now or we still have an issue?

@dmitry-gorokhov dien.xml is worked.
Problem was created in the PR - #22674
Problem is solved in the PR - #23143
Problem don't depend on current fix

@dmitry-gorokhov dmitry-gorokhov added this pull request to the merge queue Mar 14, 2024
Merged via the queue into openvinotoolkit:master with commit 5dc22c1 Mar 14, 2024
103 checks passed
alvoron pushed a commit to alvoron/openvino that referenced this pull request Apr 29, 2024
@allnes @alvoron
[ARM CPU] Fix AUGRU layer in dien.xml model (openvinotoolkit#21925)
1ad5927 
### Details:
 - *Fixed problem with precision in RNN and TensorIterator layers*
 - *Corrected shape calculation of current layers in ACL Scheduler*

### Tickets:
 - CVS-123900
 - CVS-134520
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitry-gorokhov dmitry-gorokhov dmitry-gorokhov approved these changes

@alvoron alvoron alvoron approved these changes

Assignees

@dmitry-gorokhov dmitry-gorokhov

Labels
category: CPU OpenVINO CPU plugin platform: arm OpenVINO on ARM / ARM64
Projects
None yet
Milestone
2024.1
Development

Successfully merging this pull request may close these issues.
3 participants
@allnes @alvoron @dmitry-gorokhov