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.

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

Removed constant DDR_MAX_SIZE = 512. #4372

Conversation

IvanPoliksenov
Copy link
Contributor

Removed the DDR_MAX_SIZE constant as it could potentially lead to incorrect behavior of devices with a different DDR size (Prism Creek can be up to 2 GB in size). Removed the use of this constant in methods.

@IvanPoliksenov IvanPoliksenov requested review from Maxim-Doronin and a team February 16, 2021 17:37
@IvanPoliksenov IvanPoliksenov requested a review from a team as a code owner February 16, 2021 17:37
@openvino-pushbot openvino-pushbot added category: inference OpenVINO Runtime library - Inference category: VPU labels Feb 16, 2021
Copy link
Contributor

@Maxim-Doronin Maxim-Doronin left a comment

Choose a reason for hiding this comment

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

@AndrewBakalinIntel, could you take a look as well, please

@@ -480,10 +469,6 @@ std::size_t Allocator::freeCMXMemoryAmount() const {
return _maxCmxSize - offset;
}

std::size_t Allocator::freeMemoryAmount(const MemoryType& type) const {
return type == MemoryType::CMX ? freeCMXMemoryAmount() : freeDDRMemoryAmount();
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about removing CMX check. CMX has the same capacity both for ma2480/ma2085

@@ -481,7 +470,7 @@ std::size_t Allocator::freeCMXMemoryAmount() const {
}

std::size_t Allocator::freeMemoryAmount(const MemoryType& type) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this method and leave only freeCMXMemoryAmount

@@ -77,8 +77,6 @@ void updateChildDataAllocation(const Data& data, int offsetLimitation) {
}

memoryOffset += byteOffset;

IE_ASSERT(memoryOffset + child->lastElemOffset() <= offsetLimitation);
Copy link
Contributor

Choose a reason for hiding this comment

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

You are removing an assert in CMX case
You can pass _maxCmxSize and do something like
IE_ASSERT(parent->dataLocation() != Location::CMX || memoryOffset + child->lastElemOffset() <= offsetLimitation);

@Maxim-Doronin Maxim-Doronin requested a review from a user March 12, 2021 11:16
@IvanPoliksenov IvanPoliksenov force-pushed the icv/ipolikse/rem_DDR_MAX_SIZE branch from 1881cae to 1b26a02 Compare March 12, 2021 11:32
@IvanPoliksenov IvanPoliksenov force-pushed the icv/ipolikse/rem_DDR_MAX_SIZE branch 2 times, most recently from c5645a4 to 1b26a02 Compare March 26, 2021 14:01
@@ -108,7 +110,7 @@ class Allocator final {
void extractDatas(MemoryType memType, const DataSet& from, DataVector& out) const;

std::size_t freeDDRMemoryAmount() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot to remove the declaration

namespace {

void updateChildDataAllocation(const Data& data, int offsetLimitation) {
void Allocator::updateChildDataAllocation(const Data& data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

style: no new line after }

Comment on lines 525 to 531
const auto freeSpace = (memType == MemoryType::CMX ? freeCMXMemoryAmount() : -1);
if (static_cast<std::size_t>(size) > freeSpace) {
return nullptr;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

if (memType == MemoryType::CMX && static_cast<std::size_t>(size) > freeCMXMemoryAmount()) {
    return nullptr;
}

Comment on lines 234 to 237
VPU_THROW_UNLESS(!failedData || failedData->memReqs() == MemoryType::CMX,
R"(Stage "{}" of type "{}" requested {} bytes in {} for output "{}", while there is only {} bytes is free)",
failedStage->name(), failedStage->type(), calcAllocationSize(failedData), failedData->memReqs(), failedData->name(),
allocator.freeMemoryAmount(failedData->memReqs()));
failedData->memReqs() == MemoryType::CMX ? allocator.freeCMXMemoryAmount() : -1);
Copy link
Contributor

Choose a reason for hiding this comment

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

failedData->memReqs() == MemoryType::CMX is always false in error message arguments. Take a look at the condition of assertion.
I'd suggest something like this:

VPU_THROW_UNLESS(!failedData || failedData->memReqs() == MemoryType::CMX,
R"(Request {} bytes in {} for output "{}" failed for stage "{}" of type "{}"",
calcAllocationSize(failedData), failedData->memReqs(), failedData->name(), failedStage->name(), failedStage->type());

@Maxim-Doronin
Copy link
Contributor

Please, rebase your branch on the latest master to re-trigger pre-commit validation

Removed the DDR_MAX_SIZE constant as it could potentially lead to incorrect behavior of devices with a different DDR size (Prism Creek can be up to 2 GB in size). Removed the use of this constant in methods.
Signed-off-by: Ivan Poliksenov <[email protected]>
Signed-off-by: Ivan Poliksenov <[email protected]>
@IvanPoliksenov IvanPoliksenov force-pushed the icv/ipolikse/rem_DDR_MAX_SIZE branch from e79f95f to 48f3b09 Compare April 23, 2021 09:09
@Maxim-Doronin Maxim-Doronin merged commit 2bb8e9f into openvinotoolkit:master May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: inference OpenVINO Runtime library - Inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants