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 5 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
6 changes: 5 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,8 +46,12 @@ 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");
TResult out;
if (!ptr && size == 0) {
mmikolajcz marked this conversation as resolved.
Show resolved Hide resolved
// Tensor is empty
return out;
}
OPENVINO_ASSERT(!!ptr, "ptr is Null");
auto out_it = std::inserter(out, out.end());

using namespace ov::element;
Expand Down
25 changes: 15 additions & 10 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,19 @@ 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;
if (!attrs.isSliceScatterOp) {
shapeHasDataDependency = true;
}
mmikolajcz marked this conversation as resolved.
Show resolved Hide resolved
}
}
hasConstAttrInputs = !shapeHasDataDependency;
if (isAxesSpecified)
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 +294,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 +327,12 @@ bool StridedSlice::needShapeInfer() const {
}

void StridedSlice::execute(dnnl::stream strm) {
if (!execPtr)
if (!execPtr && !Node::isDynamic) {
// SliceScatter due to not having data dependency on shape may not call prepareParams when start/stop/step values are non-constant in Static execution.
StridedSlice::prepareParams();
} else if (!execPtr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The same logic could be implemented without duplication of the !execPtr check.
As the comment is related to the SliceScatter op, shouldn't the attrs.isSliceScatterOp be checked as well?
Also is there a chance that the prepareParams will not initialize the execPtr and it will be still a nullptr after?

Suggested change
if (!execPtr && !Node::isDynamic) {
// SliceScatter due to not having data dependency on shape may not call prepareParams when start/stop/step values are non-constant in Static execution.
StridedSlice::prepareParams();
} else if (!execPtr) {
if (!execPtr) {
if (attrs.isSliceScatterOp && !isDynamicNode()) {
// SliceScatter due to not having data dependency on shape may not call prepareParams when start/stop/step values are non-constant in Static execution.
StridedSlice::prepareParams();
} else {
OPENVINO_THROW(errorPrefix, "doesn't have compiled executor!");

Is it a final solution or temp fix for the issue described in the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking for isSliceScatter op may not be needed, instead it would make more sense to check for const inputs. for start/stop/step.
For other slice ops prepareParams is called in 2 ways:

  1. If params are const then prepareParams will be called by createPrimitive,
  2. If params are not const, createPrimitive will not be called, however since Slice/StridedSlice requires values from params to determine output shape, this node will be dynamic, thus it will call prepareParams in updateDynamicParams

In SliceScatter there is a problem with option 2 because output shape doesn't depend on data. So this would be an workaround for case where all inputs would have static shapes (node is static so no updateDynamicParams) and start/stop/step would be non-constant (prepareParams cannot be called during createPrimitive due to depending on const value).

As for final solution, I guess it may benefit from some changes to StridedSliceCommonExecutor since with static shapes, some calculations could be prepared in createPrimitive stage.

OPENVINO_THROW(errorPrefix, "doesn't have compiled executor!");

}
execPtr->exec(srcMemory, dstMemory);
}

Expand Down Expand Up @@ -762,7 +767,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