-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[CPU]fix_list_iterators_incompatible #28884
base: releases/2023/3
Are you sure you want to change the base?
[CPU]fix_list_iterators_incompatible #28884
Conversation
if (loop_begin_pos == old_loop_begin_pos && loop_end_pos == old_loop_end_pos) { | ||
this->remove_loop_info(old_id); | ||
} |
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.
this part is not needed as the iterators are from different containers hence remove_loop_info
is always not executed. Comparing such iterators could trigger assert failure in debug mode and have undefined behavior in release mode.
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 believe that the comparison of iterators from different containers happens only in specific cases. If I pass loop_begin_pos = old_loop_begin_pos, loop_end_pos = old_loop_end_pos
, this code part should be executed.
I've found the my fix for this problem "comparison of iterators from different containers" - #21934. It was merged in 2024.0 release, the next release after the affected 2023.3. May I ask you to check if it's possible to port this fix to 2023.3 if the reporter can use only 2023.3?
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 just back ported the minimal changes that fix the issue. Thanks Alexandra!
@a-sidorova Could you please take a look? |
new_loop_begin_pos, | ||
new_loop_end_pos, |
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.
As far as I understand, now new_loop_begin_pos
is iterator of new LoopBegin
and new_loop_end_pos
is the next iterator after LoopEnd
(previously we passed std::next(new_loop_begin_pos)
and std::prev(new_loop_end_pos)
).
When we pass such iterators to replace_with_new_loop
, it will call replace_loop_id
for LoopBegin
- but loop-related expressions are not marked by the ID which they describe. There will be exception Expression doesn't have the Loop with ID
.
Current replace_with_new_loop
works with only expressions inside Loop-related expressions. So you need to pass inner iterators to replace_with_new_loop
as it was done before your changes. Or support work with Loop-related
expressions in replace_with_new_loop
as it's done in the fix #21934 - replace_with_new_loop
handles the case with existing Loop-related expressions and if they don't exist. Probably we can port the whole fix to 2023.3 to avoid any regressions and problems since the fix was already validated.
Also I just in case built your commit and launch Snippets CPU tests, they're failed with the same exception:
[ RUN ] smoke_Snippets_Eltwise/MaxNumParamsEltwise.CompareWithRefImpl/IS[0]=[]_TS[0]=(1.64.10.10)_#N=2_#S=1_targetDevice=CPU
MEM_USAGE=42376KB
src/tests/functional/shared_test_classes/src/base/ov_subgraph.cpp:90: Failure
Exception from src/inference/src/core.cpp:110:
[ GENERAL_ERROR ] Check 'it != loop_ids.end()' failed at src/common/snippets/src/lowered/loop_manager.cpp:575:
Expression doesn't have the Loop with ID 2
unknown file: Failure
C++ exception with description "Exception from src/inference/src/compiled_model.cpp:35:
CompiledModel was not initialized.
" thrown in the test body.
[ FAILED ] smoke_Snippets_Eltwise/MaxNumParamsEltwise.CompareWithRefImpl/IS[0]=[]_TS[0]=(1.64.10.10)_#N=2_#S=1_targetDevice=CPU, where GetParam() = (({}, { { 1, 64, 10, 10 } }), 2, 1, "CPU") (39 ms)
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.
@a-sidorova , I port the full fix, most changes(git diff) are the same between two PRs, except
- changes in
insert_loops.cpp
, as they have different base code(This is the reason why I didn't apply full fix previously). - some code style
- code explanation
Could you help take a look again? thank you!
Details: