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

[microNPU][4] Add the cascader Proposal generator #9959

Merged
merged 2 commits into from
Feb 24, 2022

Conversation

mbaret
Copy link
Contributor

@mbaret mbaret commented Jan 18, 2022

RFC: apache/tvm-rfcs#37
Issue: #9429

The Proposal generator takes optimal Plans and combines them to find optimal 'Proposals' - sets of disjoint Plans that cover every Part in a CascaderGraph. It ultimately produces a Pareto-frontier of 'optimal' Proposals in terms of estimated cycles and memory usage.

The Proposal generator takes optimal Plans and combines
them to find optimal 'Proposals' - sets of disjoint
Plans that cover every Part in a CascaderGraph. It
ultimately produces a Pareto-frontier of 'optimal'
Proposals in terms of estimated cycles and memory usage.

Change-Id: Id42099819a596496a5769bae22f08eeb75ec69b6
Change-Id: I4f5f2a298bd3bb379c7c8d179150358923b0dd66
Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

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

Thanks @mbaret .

I have let some comments; some questions, some for more docs and some for slight refactors.
Happy to take these in a follow up to allow reviews of other PRs stacked on top of this.

I think we should try to progress this PR.

std::sort(proposals.begin(), proposals.end(), [](const Proposal& a, const Proposal& b) -> bool {
return a->GetMemoryUsage() < b->GetMemoryUsage();
});
std::vector<std::array<float, 2>> costs;
Copy link
Contributor

Choose a reason for hiding this comment

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

for future : it might be better to use a typed struct here

namespace ethosu {
namespace cascader {

std::unordered_set<TensorConfig> GetPlanBoundaryConfigs(const Plan& plan) {
Copy link
Contributor

@manupak manupak Feb 16, 2022

Choose a reason for hiding this comment

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

Should we have docs for this function ?

return boundary_configs;
}

bool IsPlanCompatible(const Proposal& proposal, const std::vector<Part>& plan_part_group,
Copy link
Contributor

@manupak manupak Feb 16, 2022

Choose a reason for hiding this comment

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

Should we have docs for this function ?

return true;
}

std::unordered_map<Part, std::vector<Plan>, ObjectPtrHash, ObjectPtrEqual> CreatePlansByPart(
Copy link
Contributor

@manupak manupak Feb 16, 2022

Choose a reason for hiding this comment

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

Should we have docs for this function ?

return plans_by_part;
}

Proposal AddPlanToProposal(const Proposal& proposal, const Plan& plan,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have docs for this functions ?

return std::find(plan->GetPartGroup().begin(), plan->GetPartGroup().end(),
value) == plan->GetPartGroup().end();
});
// std::sort(residual_proposal_group.begin(), residual_proposal_group.end());
Copy link
Contributor

Choose a reason for hiding this comment

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

remove ?

const CascaderGraph& graph, const HomeMap& home_map, const CascaderOptions options,
const std::unordered_map<Part, std::vector<Plan>, ObjectPtrHash, ObjectPtrEqual>& plans_by_part,
const std::vector<Part>& partial_proposal_group,
std::unordered_map<std::vector<Part>, std::vector<Proposal>>* proposals_by_group) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, what is the reasoning to use a pointer as opposed to references elsewhere ?

value) == plan->GetPartGroup().end();
});
// std::sort(residual_proposal_group.begin(), residual_proposal_group.end());
const auto& residual_proposals = GeneratePartialProposals(
Copy link
Contributor

Choose a reason for hiding this comment

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

A comment would help here to differentiate between "residual_proposal_group" vs "residual_proposals"

value) == plan->GetPartGroup().end();
});
// std::sort(residual_proposal_group.begin(), residual_proposal_group.end());
const auto& residual_proposals = GeneratePartialProposals(
Copy link
Contributor

Choose a reason for hiding this comment

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

It also feels like constant context data is passed onto the recursive call that does not change between calls.
Should we restructure this to be a class to hold such data (e.g. graph, home_map, proposals_by_group) ?

std::unordered_map<Tensor, std::vector<MemoryRegion>, ObjectPtrHash, ObjectPtrEqual>
mhome_map;
for (const auto& it : home_map) {
std::vector<MemoryRegion> home_regions;
Copy link
Contributor

Choose a reason for hiding this comment

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

Did not we have a utility to convert Arrays to Vectors somewhere ?

@manupak
Copy link
Contributor

manupak commented Feb 23, 2022

We can unblock the reviews of the next PR, if we could address the comments of this PR in a seperate follow up PR.
WDYT @mbaret ?

@manupak manupak added the status: need update need update based on feedbacks label Feb 23, 2022
@mbaret
Copy link
Contributor Author

mbaret commented Feb 24, 2022

Yup, happy to take it as a follow up.

Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

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

LGTM! (bar the comments that is deffered to the follow up)

@manupak manupak merged commit f1ff61a into apache:main Feb 24, 2022
@manupak
Copy link
Contributor

manupak commented Feb 24, 2022

Thanks! @mbaret

pfk-beta pushed a commit to pfk-beta/tvm that referenced this pull request Apr 11, 2022
* [microNPU][4] Add the cascader Proposal generator

The Proposal generator takes optimal Plans and combines
them to find optimal 'Proposals' - sets of disjoint
Plans that cover every Part in a CascaderGraph. It
ultimately produces a Pareto-frontier of 'optimal'
Proposals in terms of estimated cycles and memory usage.

Change-Id: Id42099819a596496a5769bae22f08eeb75ec69b6

* Fixes

Change-Id: I4f5f2a298bd3bb379c7c8d179150358923b0dd66
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: need update need update based on feedbacks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants