-
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
[NPU] Switching the I/O identification convention to indices #24248
[NPU] Switching the I/O identification convention to indices #24248
Conversation
329edac
to
c904368
Compare
828b305
to
9d65acc
Compare
17045cf
to
5017159
Compare
5017159
to
4432a71
Compare
f53d917
to
f384e88
Compare
struct IONodeDescriptor { | ||
std::string legacyName; | ||
std::string currentNodeName; | ||
struct IODescriptor { |
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.
Most of the changes snowballed from here.
ioDescriptor.nameFromCompiler, | ||
" vs. ", | ||
zeDescriptorName, | ||
". The I/O order may have been altered, which could lead to an erroneous behavior."); |
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.
Safeguard - checks if the driver has altered the I/O order.
|
||
return std::nullopt; | ||
return candidateBatchSize; | ||
} |
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 admit refactoring this one wasn't necessary. I ended up writting this in another context, then I've decided to move it here and replace the older function. I'm leaning towards this implementation for several minor reasons:
- Previously, the logger verbosity level was not handled correctly.
- The current implementation is not relying on Level Zero specific objects.
- Fail fast approach - slightly more efficient I believe.
if (inputDescriptor.isShapeTensor) { | ||
OPENVINO_ASSERT(inputDescriptor.relatedDescriptorIndex.has_value(), | ||
"The link between the dynamic tensor and its shape tensor is missing, entry name: ", | ||
inputDescriptor.nameFromCompiler); |
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.
See bindRelatedDescriptors()
inside icompiler.hpp
for more details on how these links are established.
IONodeDescriptorMap& results, | ||
IONodeDescriptorMap& states, | ||
std::vector<std::string>& stateNames, | ||
const T& arg) const; |
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.
We're not sending layouts anymore (other than the default one), so it's safe to assume extracting the layout is now a redundant operation.
// Flag used for indicating an NPU plugin version which switched the I/O identification convention from names to | ||
// indices. The flag is required in order to inform the driver-compiler adapter to expect indices when attempting to | ||
// deserialize the I/O metadata. | ||
const auto useIndicesForIOMetadata = "use_indices_for_io_metadata"; |
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.
No plugin versioning unfortunately, we have to rely on flags for signaling custom behavior inside the driver-compiler adapter.
}(); | ||
std::shared_ptr<ov::op::v0::Parameter> parameter = std::make_shared<ov::op::v0::Parameter>( | ||
inputDescriptor.precision, | ||
inputDescriptor.shapeFromIRModel.has_value() ? *inputDescriptor.shapeFromIRModel |
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.
shapeFromIRModel
may have no value if using an old driver in the CiD scenario. See NotSupportArgumentMetadata
inside zero_compiler_in_driver.hpp
.
for (const IODescriptor& outputDescriptor : outputDescriptors) { | ||
if (outputDescriptor.isStateInput || outputDescriptor.isStateOutput || outputDescriptor.isShapeTensor) { | ||
continue; | ||
} |
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.
States/shape tensors are added by the compile as I/O, these are not part of the original IR model.
@@ -0,0 +1,118 @@ | |||
// Copyright (C) 2018-2024 Intel Corporation |
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 lemme know if there's a better place for this test.
21a3990
to
c8a4132
Compare
|
||
namespace { | ||
|
||
INSTANTIATE_TEST_SUITE_P(smoke_BehaviorTests, |
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.
Although not under the behavior/
directory, I've chosen the smoke_BehaviorTests
in order to allow the test to fall under one of the validation jobs regexes.
mutable std::unordered_map<std::string, std::shared_ptr<ov::ITensor>> _shapesTensors; | ||
// A copy of each tensor is needed to maintain the original L0 memory allocation in case the user provides another | ||
// memory area for the tensor. | ||
std::unordered_map<std::string, std::shared_ptr<ov::ITensor>> _copyAllTensors; |
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.
Moved to zero_infer_request.hpp
as _levelZeroInputTensors
and _levelZeroOutputTensors
since only the L0 backend was using this attribute.
73f1fd0
to
d78083d
Compare
d78083d
to
76dbce1
Compare
76dbce1
to
3b8d69b
Compare
ecf8093
to
d9334df
Compare
a24ee00
to
860552c
Compare
Details:
Tickets: