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

[OP][CPU] Fix SliceScatter issues with non-constant slice params #27482

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from
+55 −19
Draft

[OP][CPU] Fix SliceScatter issues with non-constant slice params #27482

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
774582c
Fix SliceScatter issue with non-constant slice params and static exec…
mmikolajcz Nov 8, 2024
4f9abba
Fix issue with calling get_raw_data_as on empty tensor
mmikolajcz Nov 8, 2024
e1cf06f
Add missing reference template tests with 0-dim slice params
mmikolajcz Nov 8, 2024
8999701
Merge branch 'master' into mateuszm/slicescatter/cpu_bugfix
mmikolajcz Nov 15, 2024
f86bd87
Merge branch 'master' into mateuszm/slicescatter/cpu_bugfix
mmikolajcz Nov 22, 2024
a74d9ae
Fix retrieving raw tensor data for size 0 tensor
mmikolajcz Nov 27, 2024
5170814
Apply requested changes
mmikolajcz Nov 28, 2024
a4bcad3
Merge branch 'master' into mateuszm/slicescatter/cpu_bugfix
mmikolajcz Nov 28, 2024
e355673
Merge branch 'master' into mateuszm/slicescatter/cpu_bugfix
mmikolajcz Nov 29, 2024
26f0a65
Merge branch 'master' into mateuszm/slicescatter/cpu_bugfix
mmikolajcz Dec 4, 2024
e82524f
Remove unused var
mmikolajcz Dec 11, 2024
cf599f0
Merge branch 'master' of https://github.com/openvinotoolkit/openvino …
mmikolajcz Dec 18, 2024
58bfb56
Fix formatting
mmikolajcz Dec 18, 2024
27e625e
Merge branch 'master' into mateuszm/slicescatter/cpu_bugfix
mmikolajcz Jan 3, 2025
26b47f0
Merge branch 'master' into mateuszm/slicescatter/cpu_bugfix
mmikolajcz Jan 16, 2025
aaeedc8
Merge branch 'master' into mateuszm/slicescatter/cpu_bugfix
mmikolajcz Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/core/shape_inference/include/utils.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ struct TensorTransform : element::NotSupported<void> {
*/
template <class T, class TResult = std::vector<T>, class UnaryOperation>
TResult get_raw_data_as(const element::Type_t et, const void* const ptr, const size_t size, UnaryOperation&& func) {
OPENVINO_ASSERT(!!ptr, "ptr is Null");
// Based on OV ViewTensor, size 0 tensor is allowed to have nullptr memory ptr.
OPENVINO_ASSERT(size == 0 || ptr != nullptr, "ptr is Null");
TResult out;
auto out_it = std::inserter(out, out.end());

Expand Down
28 changes: 17 additions & 11 deletions src/plugins/intel_cpu/src/nodes/strided_slice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,17 +71,17 @@ StridedSlice::StridedSlice(const std::shared_ptr<ov::Node>& op, const GraphConte
if (inputShapes.size() > attrs.AXES_ID) {
isAxesSpecified = true;
}

hasConstAttrInputs = true;
for (size_t i = 0lu; i < op->get_input_size(); i++) {
isConstantInput[i] = ov::is_type<ov::op::v0::Constant>(op->get_input_node_shared_ptr(i));
if (!isConstantInput[i] && one_of(i, attrs.BEGIN_ID, attrs.END_ID, attrs.STRIDE_ID) && !attrs.isSliceScatterOp) {
shapeHasDataDependency = true;
if (!isConstantInput[i] && one_of(i, attrs.BEGIN_ID, attrs.END_ID, attrs.STRIDE_ID)) {
hasConstAttrInputs = false;
}
}
hasConstAttrInputs = !shapeHasDataDependency;
if (isAxesSpecified)
shapeHasDataDependency = attrs.isSliceScatterOp ? false : (!hasConstAttrInputs);
if (isAxesSpecified) {
hasConstAttrInputs &= isConstantInput[attrs.AXES_ID];

}
const size_t inputRank = getInputShapeAtPort(attrs.DATA_ID).getRank();
const size_t outputRank = getOutputShapeAtPort(0).getRank();
const size_t nDims = std::max(inputRank, outputRank);
Expand Down Expand Up @@ -292,7 +292,7 @@ bool StridedSlice::isExecutable() const {
}

void StridedSlice::createPrimitive() {
if (inputShapesDefined() && isExecutable() && !shapeHasDataDependency) {
if (inputShapesDefined() && isExecutable() && !shapeHasDataDependency && hasConstAttrInputs) {
if (needPrepareParams()) {
prepareParams();
}
Expand Down Expand Up @@ -325,9 +325,15 @@ bool StridedSlice::needShapeInfer() const {
}

void StridedSlice::execute(dnnl::stream strm) {
if (!execPtr)
OPENVINO_THROW(errorPrefix, "doesn't have compiled executor!");

if (!execPtr) {
if (!isDynamicNode() && !hasConstAttrInputs) {
// SliceScatter due to not having data dependency on shape may not call prepareParams when start/stop/step values are non-constant in Static execution.
// In Slice and SliceScatter op, prepareParams would be called by createPrimitive (if const inputs) or by updateDynamicParams in case of dynamic node.
StridedSlice::prepareParams();
} else {
OPENVINO_THROW(errorPrefix, "doesn't have compiled executor!");
}
}
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 data on the corresponding inputs are not constant, it means that the executor must be reinitialized every inference call, as the data in variables may be updated each inference. If the tests pass, that means that either the test converge is still not sufficient, or there is no real data dependency and the executor may be created at the compilation stage.

execPtr->exec(srcMemory, dstMemory);
}

Expand Down Expand Up @@ -762,7 +768,7 @@ void StridedSlice::StridedSliceCommonExecutor::execSliceScatter(const std::vecto
const uint8_t* srcUpdates = srcMemory[1]->getDataAs<const uint8_t>();
uint8_t* dstData = dstMemory[0]->getDataAs<uint8_t>();
cpu_parallel_memcpy(dstData, srcData, srcMemory[0]->getSize());
if (srcMemory[1]->getSize() == 0) {
if (srcMemory[1]->getShape().hasZeroDims() || srcMemory[1]->getSize() == 0) {
// Updates are empty - do not apply
return;
}
Expand Down
16 changes: 11 additions & 5 deletions src/plugins/intel_cpu/tests/functional/custom/single_layer_tests/slice_scatter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,13 @@ class SliceScatterLayerCPUTest : public testing::WithParamInterface<SliceScatter
in_data);
} else {
// Fill the slice input2~input5 with specified data.
tensor = ov::Tensor{ov::element::i64, targetInputStaticShapes[i], inputValues[i - 2]};
auto inputValue = inputValues[i - 2];
if (!inputValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please elaborate on the reason behind introducing this check? It looks like the current test configuration doesn't provide null pointers. But if it does, an input tensor is created but not initialized with valid values. So what is the expectation of using such an input?

const auto param = ov::as_type_ptr<const ov::op::v0::Parameter>(funcInput.get_node_shared_ptr());
mmikolajcz marked this conversation as resolved.
Show resolved Hide resolved
tensor = ov::Tensor{ov::element::i64, targetInputStaticShapes[i]};
} else {
tensor = ov::Tensor{ov::element::i64, targetInputStaticShapes[i], inputValue};
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually this is the reason why the tests don't cover the main issue, as is being resolved in this PR. The thing is that the SL tests in the static shapes configuration perform only one inference, thus the values on the variable inputs don't really change from infer to infer and SL tests don't spot the issue. But if we had had such tests, they would have revealed the design flaw of the current solution solution (please see the comment regarding execute method implementation). So apparently the SL tests should be extended to cover such a use case: the values on the variable inputs actually changes each inference. To this end even a dedicated test class may be introduced, when necessary.

}
}
inputs.insert({funcInput.get_node_shared_ptr(), tensor});
}
Expand Down Expand Up @@ -228,7 +234,7 @@ INSTANTIATE_TEST_SUITE_P(smoke_CompareWithRefs_Plain_Static_2D,
SliceScatterLayerCPUTest,
::testing::Combine(::testing::Values(static_shapes_to_test_representation({{32, 16}})),
::testing::ValuesIn(paramsPlain2D),
::testing::Values(ov::test::utils::InputLayerType::CONSTANT),
::testing::ValuesIn(inputLayerTypes),
::testing::ValuesIn(inputPrecisions),
::testing::Values(emptyCPUSpec)),
SliceScatterLayerCPUTest::getTestCaseName);
Expand Down Expand Up @@ -319,7 +325,7 @@ INSTANTIATE_TEST_SUITE_P(
SliceScatterLayerCPUTest,
::testing::Combine(::testing::ValuesIn(static_shapes_to_test_representation(inputShapesStatic4D)),
::testing::ValuesIn(testCasesCommon4D),
::testing::Values(ov::test::utils::InputLayerType::CONSTANT),
::testing::ValuesIn(inputLayerTypes),
::testing::ValuesIn(inputPrecisions),
::testing::ValuesIn(CPUParamsCommon4D)),
SliceScatterLayerCPUTest::getTestCaseName);
Expand Down Expand Up @@ -407,7 +413,7 @@ INSTANTIATE_TEST_SUITE_P(
SliceScatterLayerCPUTest,
::testing::Combine(::testing::ValuesIn(static_shapes_to_test_representation(inputShapesStatic5D)),
::testing::ValuesIn(testCasesCommon5D),
::testing::Values(ov::test::utils::InputLayerType::CONSTANT),
::testing::ValuesIn(inputLayerTypes),
::testing::ValuesIn(inputPrecisions),
::testing::ValuesIn(CPUParamsCommon5D)),
SliceScatterLayerCPUTest::getTestCaseName);
Expand Down Expand Up @@ -441,7 +447,7 @@ INSTANTIATE_TEST_SUITE_P(smoke_CompareWithRefs_Common_Full_Slice_5D,
SliceScatterLayerCPUTest,
::testing::Combine(::testing::ValuesIn(inputShapesFullSlice5D),
::testing::ValuesIn(testCasesFullSlice5D),
::testing::Values(ov::test::utils::InputLayerType::CONSTANT),
::testing::ValuesIn(inputLayerTypes),
::testing::ValuesIn(inputPrecisions),
::testing::ValuesIn(CPUParamsCommon5D)),
SliceScatterLayerCPUTest::getTestCaseName);
Expand Down
24 changes: 24 additions & 0 deletions src/plugins/template/tests/functional/op_reference/slice_scatter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,13 @@ std::vector<SliceScatterParams> generateSliceScatterParamsUnsigned() {
reference_tests::Tensor{{1}, AXIS_ET, std::vector<AXIS_T>{0}},
reference_tests::Tensor{{4}, DATA_ET, std::vector<DATA_T>{1, 10, 3, 20}},
"1D_2_step_replace"),
SliceScatterParams(reference_tests::Tensor{{4}, DATA_ET, std::vector<DATA_T>{1, 2, 3, 4}},
reference_tests::Tensor{{4}, DATA_ET, std::vector<DATA_T>{10, 20, 30, 40}},
reference_tests::Tensor{{0}, IND_ET, std::vector<IND_T>{}},
reference_tests::Tensor{{0}, IND_ET, std::vector<IND_T>{}},
reference_tests::Tensor{{0}, IND_ET, std::vector<IND_T>{}},
reference_tests::Tensor{{4}, DATA_ET, std::vector<DATA_T>{10, 20, 30, 40}},
"1D_full_replace_empty_slice_params"),
SliceScatterParams(
reference_tests::Tensor{{4, 4},
DATA_ET,
Expand Down Expand Up @@ -348,6 +355,23 @@ std::vector<SliceScatterParams> generateSliceScatterParamsUnsigned() {
30, 31, 32, 33, 34, 35, 54, 55, 38, 39,
40, 41, 56, 57, 44, 45, 46, 47}},
"4D_partial_replace_even_axes"),
SliceScatterParams(
reference_tests::Tensor{{4, 2, 3, 2}, DATA_ET, std::vector<DATA_T>{}},
reference_tests::Tensor{{4, 2, 3, 2}, DATA_ET, std::vector<DATA_T>{0, 1, 2, 3, 4, 5, 6, 7, 8, 9,
10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
20, 21, 22, 23, 24, 25, 26, 27, 28, 29,
30, 31, 32, 33, 34, 35, 36, 37, 38, 39,
40, 41, 42, 43, 44, 45, 46, 47}},
reference_tests::Tensor{{0}, IND_ET, std::vector<IND_T>{}},
reference_tests::Tensor{{0}, IND_ET, std::vector<IND_T>{}},
reference_tests::Tensor{{0}, IND_ET, std::vector<IND_T>{}},
reference_tests::Tensor{{0}, IND_ET, std::vector<AXIS_T>{}},
reference_tests::Tensor{{4, 2, 3, 2}, DATA_ET, std::vector<DATA_T>{0, 1, 2, 3, 4, 5, 6, 7, 8, 9,
10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
20, 21, 22, 23, 24, 25, 26, 27, 28, 29,
30, 31, 32, 33, 34, 35, 36, 37, 38, 39,
40, 41, 42, 43, 44, 45, 46, 47}},
"4D_full_replace_empty_slice_params"),
};
return test_params;
}
Expand Down
Loading